| Message ID | 20220121152350.381685-1-fbarrat@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/pnv: Fail DMA access if page permissions are not correct | expand |
On 1/21/22 12:23, Frederic Barrat wrote: > If an iommu page has wrong permissions, an error message is displayed, > but the access is allowed, which is odd. This patch fixes it. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > hw/pci-host/pnv_phb3.c | 11 ++++++----- > hw/pci-host/pnv_phb4.c | 11 ++++++----- > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c > index 7fb35dc031..a757f1a58e 100644 > --- a/hw/pci-host/pnv_phb3.c > +++ b/hw/pci-host/pnv_phb3.c > @@ -816,18 +816,19 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr, > } > > /* We exit the loop with TCE being the final TCE */ > - tce_mask = ~((1ull << tce_shift) - 1); > - tlb->iova = addr & tce_mask; > - tlb->translated_addr = tce & tce_mask; > - tlb->addr_mask = ~tce_mask; > - tlb->perm = tce & 3; > if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { > phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr); > phb3_error(phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, > is_write ? 'W' : 'R', tve); > phb3_error(phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", > tta, lev, tts, tps); > + return; > } > + tce_mask = ~((1ull << tce_shift) - 1); > + tlb->iova = addr & tce_mask; > + tlb->translated_addr = tce & tce_mask; > + tlb->addr_mask = ~tce_mask; > + tlb->perm = tce & 3; > } > } > > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index a78add75b0..ee56377c02 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -1291,18 +1291,19 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr, > } > > /* We exit the loop with TCE being the final TCE */ > - tce_mask = ~((1ull << tce_shift) - 1); > - tlb->iova = addr & tce_mask; > - tlb->translated_addr = tce & tce_mask; > - tlb->addr_mask = ~tce_mask; > - tlb->perm = tce & 3; > if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { > phb_error(ds->phb, "TCE access fault at 0x%"PRIx64, taddr); > phb_error(ds->phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, > is_write ? 'W' : 'R', tve); > phb_error(ds->phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", > tta, lev, tts, tps); > + return; > } > + tce_mask = ~((1ull << tce_shift) - 1); > + tlb->iova = addr & tce_mask; > + tlb->translated_addr = tce & tce_mask; > + tlb->addr_mask = ~tce_mask; > + tlb->perm = tce & 3; > } > } >
On 1/21/22 16:23, Frederic Barrat wrote: > If an iommu page has wrong permissions, an error message is displayed, > but the access is allowed, which is odd. This patch fixes it. Being curious. How do you generate such errors ? could we have the output ? Thanks, C. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- > hw/pci-host/pnv_phb3.c | 11 ++++++----- > hw/pci-host/pnv_phb4.c | 11 ++++++----- > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c > index 7fb35dc031..a757f1a58e 100644 > --- a/hw/pci-host/pnv_phb3.c > +++ b/hw/pci-host/pnv_phb3.c > @@ -816,18 +816,19 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr, > } > > /* We exit the loop with TCE being the final TCE */ > - tce_mask = ~((1ull << tce_shift) - 1); > - tlb->iova = addr & tce_mask; > - tlb->translated_addr = tce & tce_mask; > - tlb->addr_mask = ~tce_mask; > - tlb->perm = tce & 3; > if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { > phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr); > phb3_error(phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, > is_write ? 'W' : 'R', tve); > phb3_error(phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", > tta, lev, tts, tps); > + return; > } > + tce_mask = ~((1ull << tce_shift) - 1); > + tlb->iova = addr & tce_mask; > + tlb->translated_addr = tce & tce_mask; > + tlb->addr_mask = ~tce_mask; > + tlb->perm = tce & 3; > } > } > > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c > index a78add75b0..ee56377c02 100644 > --- a/hw/pci-host/pnv_phb4.c > +++ b/hw/pci-host/pnv_phb4.c > @@ -1291,18 +1291,19 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr, > } > > /* We exit the loop with TCE being the final TCE */ > - tce_mask = ~((1ull << tce_shift) - 1); > - tlb->iova = addr & tce_mask; > - tlb->translated_addr = tce & tce_mask; > - tlb->addr_mask = ~tce_mask; > - tlb->perm = tce & 3; > if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { > phb_error(ds->phb, "TCE access fault at 0x%"PRIx64, taddr); > phb_error(ds->phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, > is_write ? 'W' : 'R', tve); > phb_error(ds->phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", > tta, lev, tts, tps); > + return; > } > + tce_mask = ~((1ull << tce_shift) - 1); > + tlb->iova = addr & tce_mask; > + tlb->translated_addr = tce & tce_mask; > + tlb->addr_mask = ~tce_mask; > + tlb->perm = tce & 3; > } > } > >
On 21/01/2022 18:35, Cédric Le Goater wrote: > On 1/21/22 16:23, Frederic Barrat wrote: >> If an iommu page has wrong permissions, an error message is displayed, >> but the access is allowed, which is odd. This patch fixes it. > > > Being curious. How do you generate such errors ? could we have the > output ? By acking the code building the tce table to remove permissions on the page. See pnv_tce_build(). Also, on bare metal, device drivers using 64-bit dma will hit a translation entry (TVE#1) configured in bypass mode. So to avoid that and force translation, you need to add "iommu=nobypass" to the kernel boot args. Then you will see: phb4[0:1]: TCE access fault at 0x2a510080 phb4[0:1]: xlate 100000:R TVE=32c702505 phb4[0:1]: tta=32c70 lev=-2 tts=5 tps=5 Invalid read at addr 0x100000, size 4, region '(null)', reason: rejected P8 is more complicated... Fred > > Thanks, > > C. > > >> >> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> >> --- >> hw/pci-host/pnv_phb3.c | 11 ++++++----- >> hw/pci-host/pnv_phb4.c | 11 ++++++----- >> 2 files changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c >> index 7fb35dc031..a757f1a58e 100644 >> --- a/hw/pci-host/pnv_phb3.c >> +++ b/hw/pci-host/pnv_phb3.c >> @@ -816,18 +816,19 @@ static void >> pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr, >> } >> /* We exit the loop with TCE being the final TCE */ >> - tce_mask = ~((1ull << tce_shift) - 1); >> - tlb->iova = addr & tce_mask; >> - tlb->translated_addr = tce & tce_mask; >> - tlb->addr_mask = ~tce_mask; >> - tlb->perm = tce & 3; >> if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { >> phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr); >> phb3_error(phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, >> is_write ? 'W' : 'R', tve); >> phb3_error(phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", >> tta, lev, tts, tps); >> + return; >> } >> + tce_mask = ~((1ull << tce_shift) - 1); >> + tlb->iova = addr & tce_mask; >> + tlb->translated_addr = tce & tce_mask; >> + tlb->addr_mask = ~tce_mask; >> + tlb->perm = tce & 3; >> } >> } >> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c >> index a78add75b0..ee56377c02 100644 >> --- a/hw/pci-host/pnv_phb4.c >> +++ b/hw/pci-host/pnv_phb4.c >> @@ -1291,18 +1291,19 @@ static void >> pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr, >> } >> /* We exit the loop with TCE being the final TCE */ >> - tce_mask = ~((1ull << tce_shift) - 1); >> - tlb->iova = addr & tce_mask; >> - tlb->translated_addr = tce & tce_mask; >> - tlb->addr_mask = ~tce_mask; >> - tlb->perm = tce & 3; >> if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { >> phb_error(ds->phb, "TCE access fault at 0x%"PRIx64, taddr); >> phb_error(ds->phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, >> is_write ? 'W' : 'R', tve); >> phb_error(ds->phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", >> tta, lev, tts, tps); >> + return; >> } >> + tce_mask = ~((1ull << tce_shift) - 1); >> + tlb->iova = addr & tce_mask; >> + tlb->translated_addr = tce & tce_mask; >> + tlb->addr_mask = ~tce_mask; >> + tlb->perm = tce & 3; >> } >> } >> >
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index 7fb35dc031..a757f1a58e 100644 --- a/hw/pci-host/pnv_phb3.c +++ b/hw/pci-host/pnv_phb3.c @@ -816,18 +816,19 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr, } /* We exit the loop with TCE being the final TCE */ - tce_mask = ~((1ull << tce_shift) - 1); - tlb->iova = addr & tce_mask; - tlb->translated_addr = tce & tce_mask; - tlb->addr_mask = ~tce_mask; - tlb->perm = tce & 3; if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr); phb3_error(phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, is_write ? 'W' : 'R', tve); phb3_error(phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", tta, lev, tts, tps); + return; } + tce_mask = ~((1ull << tce_shift) - 1); + tlb->iova = addr & tce_mask; + tlb->translated_addr = tce & tce_mask; + tlb->addr_mask = ~tce_mask; + tlb->perm = tce & 3; } } diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index a78add75b0..ee56377c02 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -1291,18 +1291,19 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr, } /* We exit the loop with TCE being the final TCE */ - tce_mask = ~((1ull << tce_shift) - 1); - tlb->iova = addr & tce_mask; - tlb->translated_addr = tce & tce_mask; - tlb->addr_mask = ~tce_mask; - tlb->perm = tce & 3; if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { phb_error(ds->phb, "TCE access fault at 0x%"PRIx64, taddr); phb_error(ds->phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, is_write ? 'W' : 'R', tve); phb_error(ds->phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", tta, lev, tts, tps); + return; } + tce_mask = ~((1ull << tce_shift) - 1); + tlb->iova = addr & tce_mask; + tlb->translated_addr = tce & tce_mask; + tlb->addr_mask = ~tce_mask; + tlb->perm = tce & 3; } }
If an iommu page has wrong permissions, an error message is displayed, but the access is allowed, which is odd. This patch fixes it. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- hw/pci-host/pnv_phb3.c | 11 ++++++----- hw/pci-host/pnv_phb4.c | 11 ++++++----- 2 files changed, 12 insertions(+), 10 deletions(-)