diff mbox series

[2/2] hw/arm/smmu-common: Fix TTB1 handling

Message ID 20230210163731.970130-3-jean-philippe@linaro.org (mailing list archive)
State New, archived
Headers show
Series [1/2] hw/arm/smmu-common: Support 64-bit addresses | expand

Commit Message

Jean-Philippe Brucker Feb. 10, 2023, 4:37 p.m. UTC
Addresses targeting the second translation table (TTB1) in the SMMU have
all upper bits set (except for the top byte when TBI is enabled). Fix
the TTB1 check.

Reported-by: Ola Hugosson <ola.hugosson@arm.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/smmu-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Richard Henderson Feb. 11, 2023, 11:32 p.m. UTC | #1
On 2/10/23 06:37, Jean-Philippe Brucker wrote:
> Addresses targeting the second translation table (TTB1) in the SMMU have
> all upper bits set (except for the top byte when TBI is enabled). Fix
> the TTB1 check.
> 
> Reported-by: Ola Hugosson<ola.hugosson@arm.com>
> Signed-off-by: Jean-Philippe Brucker<jean-philippe@linaro.org>
> ---
>   hw/arm/smmu-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Eric Auger Feb. 13, 2023, 4:30 p.m. UTC | #2
Hi Jean,

On 2/10/23 17:37, Jean-Philippe Brucker wrote:
> Addresses targeting the second translation table (TTB1) in the SMMU have
> all upper bits set (except for the top byte when TBI is enabled). Fix
> the TTB1 check.
>
> Reported-by: Ola Hugosson <ola.hugosson@arm.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/smmu-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 2b8c67b9a1..0a5a60ca1e 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>          /* there is a ttbr0 region and we are in it (high bits all zero) */
>          return &cfg->tt[0];
>      } else if (cfg->tt[1].tsz &&
> -           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
> +        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
>          /* there is a ttbr1 region and we are in it (high bits all one) */
>          return &cfg->tt[1];
>      } else if (!cfg->tt[0].tsz) {

Reviewed-by: Eric Auger <eric.auger@redhat.com>

While reading the spec again, I noticed we do not support VAX. Is it
something that we would need to support?

Thanks!

Eric
Jean-Philippe Brucker Feb. 14, 2023, 4:46 p.m. UTC | #3
On Mon, Feb 13, 2023 at 05:30:03PM +0100, Eric Auger wrote:
> Hi Jean,
> 
> On 2/10/23 17:37, Jean-Philippe Brucker wrote:
> > Addresses targeting the second translation table (TTB1) in the SMMU have
> > all upper bits set (except for the top byte when TBI is enabled). Fix
> > the TTB1 check.
> >
> > Reported-by: Ola Hugosson <ola.hugosson@arm.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  hw/arm/smmu-common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> > index 2b8c67b9a1..0a5a60ca1e 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> >          /* there is a ttbr0 region and we are in it (high bits all zero) */
> >          return &cfg->tt[0];
> >      } else if (cfg->tt[1].tsz &&
> > -           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
> > +        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
> >          /* there is a ttbr1 region and we are in it (high bits all one) */
> >          return &cfg->tt[1];
> >      } else if (!cfg->tt[0].tsz) {
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> While reading the spec again, I noticed we do not support VAX. Is it
> something that we would need to support?

I guess it would be needed to support sharing page tables with the CPU, if
the CPU supports and the OS uses FEAT_LVA. But in order to share the
stage-1, Linux would need more complex features as well (ATS+PRI/Stall,
PASID).

For a private DMA address space, I think 48 bits of VA is already plenty.

Thanks,
Jean
Eric Auger Feb. 14, 2023, 6:36 p.m. UTC | #4
On 2/14/23 17:46, Jean-Philippe Brucker wrote:
> On Mon, Feb 13, 2023 at 05:30:03PM +0100, Eric Auger wrote:
>> Hi Jean,
>>
>> On 2/10/23 17:37, Jean-Philippe Brucker wrote:
>>> Addresses targeting the second translation table (TTB1) in the SMMU have
>>> all upper bits set (except for the top byte when TBI is enabled). Fix
>>> the TTB1 check.
>>>
>>> Reported-by: Ola Hugosson <ola.hugosson@arm.com>
>>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>> ---
>>>  hw/arm/smmu-common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>> index 2b8c67b9a1..0a5a60ca1e 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -249,7 +249,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
>>>          /* there is a ttbr0 region and we are in it (high bits all zero) */
>>>          return &cfg->tt[0];
>>>      } else if (cfg->tt[1].tsz &&
>>> -           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
>>> +        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
>>>          /* there is a ttbr1 region and we are in it (high bits all one) */
>>>          return &cfg->tt[1];
>>>      } else if (!cfg->tt[0].tsz) {
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>
>> While reading the spec again, I noticed we do not support VAX. Is it
>> something that we would need to support?
> I guess it would be needed to support sharing page tables with the CPU, if
> the CPU supports and the OS uses FEAT_LVA. But in order to share the
> stage-1, Linux would need more complex features as well (ATS+PRI/Stall,
> PASID).
>
> For a private DMA address space, I think 48 bits of VA is already plenty.

OK thanks!

Eric
>
> Thanks,
> Jean
>
diff mbox series

Patch

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 2b8c67b9a1..0a5a60ca1e 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -249,7 +249,7 @@  SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
         /* there is a ttbr0 region and we are in it (high bits all zero) */
         return &cfg->tt[0];
     } else if (cfg->tt[1].tsz &&
-           !extract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte)) {
+        sextract64(iova, 64 - cfg->tt[1].tsz, cfg->tt[1].tsz - tbi_byte) == -1) {
         /* there is a ttbr1 region and we are in it (high bits all one) */
         return &cfg->tt[1];
     } else if (!cfg->tt[0].tsz) {