hw/arm/smmuv3: fix smmu emulation when guest smmu is in passthrough mode

[Qemu-devel] [PATCH] hw/arm/smmuv3: fix smmu emulation when guest smmu is in passthrough mode

Posted by Jia He 5 years, 10 months ago

There is an exception when I passes iommu.passthrough=1 to guest's
kernel boot parameter(host QDF2400 kernel 4.17, guest kernel 4.14).
The guest will be hang when booting up.

When guest smmu is in passthrough mode, entry.perm will not be assigned
to flag in smmuv3_translate. It seems not be correct.

After this patch, I have tested in 4 cases and all passed.
host smmu on/passthrough + guest smmu on/passthrough

Signed-off-by: jia.he@hxt-semitech.com
---
 hw/arm/smmuv3.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 42dc521..5c46102 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -560,6 +560,12 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     }
 
     ret = smmuv3_decode_config(mr, &cfg, &event);
+
+    if (cfg.bypassed) {
+        ret = 0;
+        goto out;
+    }
+
     if (ret) {
         goto out;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/arm/smmuv3: fix smmu emulation when guest smmu is in passthrough mode

Posted by Peter Maydell 5 years, 10 months ago

On 7 June 2018 at 14:38, Jia He <hejianet@gmail.com> wrote:
> There is an exception when I passes iommu.passthrough=1 to guest's
> kernel boot parameter(host QDF2400 kernel 4.17, guest kernel 4.14).
> The guest will be hang when booting up.
>
> When guest smmu is in passthrough mode, entry.perm will not be assigned
> to flag in smmuv3_translate. It seems not be correct.
>
> After this patch, I have tested in 4 cases and all passed.
> host smmu on/passthrough + guest smmu on/passthrough
>
> Signed-off-by: jia.he@hxt-semitech.com
> ---
>  hw/arm/smmuv3.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 42dc521..5c46102 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -560,6 +560,12 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      }
>
>      ret = smmuv3_decode_config(mr, &cfg, &event);
> +
> +    if (cfg.bypassed) {
> +        ret = 0;
> +        goto out;
> +    }
> +
>      if (ret) {
>          goto out;
>      }
> --
> 1.8.3.1
>

Ccing Eric as the author of the smmuv3 code.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/smmuv3: fix smmu emulation when guest smmu is in passthrough mode

Posted by Auger Eric 5 years, 10 months ago

Hi Jia,

On 06/07/2018 03:38 PM, Jia He wrote:
> There is an exception when I passes iommu.passthrough=1 to guest's
> kernel boot parameter(host QDF2400 kernel 4.17, guest kernel 4.14).
> The guest will be hang when booting up.
> 
> When guest smmu is in passthrough mode, entry.perm will not be assigned
> to flag in smmuv3_translate. It seems not be correct.
> 
> After this patch, I have tested in 4 cases and all passed.
> host smmu on/passthrough + guest smmu on/passthrough
> 
> Signed-off-by: jia.he@hxt-semitech.com
> ---
>  hw/arm/smmuv3.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 42dc521..5c46102 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -560,6 +560,12 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      }
>  
>      ret = smmuv3_decode_config(mr, &cfg, &event);
> +
> +    if (cfg.bypassed) {
> +        ret = 0;
> +        goto out;
> +    }
Thank you for spotting this bug. Yes you're correct: on bypass we
effectively need to set the IOMMUTLBEntry perm flags.

Reading the code again I think the error handling logic is really
confusing and if you don't mind, I suggest I submit a global fix. On
bypass we should rather have a bypass trace event issued instead of the
translated one. To me the aborted case is not properly handled either as
we are going to record an event whereas we shouldn't. In case of
abort/bypass translation structure decoding should rather return 0 I
think instead of returning errors. Please let me know if it suits you.

Thanks

Eric
> +
>      if (ret) {
>          goto out;
>      }
>

Re: [Qemu-devel] [PATCH] hw/arm/smmuv3: fix smmu emulation when guest smmu is in passthrough mode

Posted by Jia He 5 years, 10 months ago

Hi Eric

On 6/8/2018 1:06 AM, Auger Eric Wrote:
> Hi Jia,
> 
> On 06/07/2018 03:38 PM, Jia He wrote:
>> There is an exception when I passes iommu.passthrough=1 to guest's
>> kernel boot parameter(host QDF2400 kernel 4.17, guest kernel 4.14).
>> The guest will be hang when booting up.
>>
>> When guest smmu is in passthrough mode, entry.perm will not be assigned
>> to flag in smmuv3_translate. It seems not be correct.
>>
>> After this patch, I have tested in 4 cases and all passed.
>> host smmu on/passthrough + guest smmu on/passthrough
>>
>> Signed-off-by: jia.he@hxt-semitech.com
>> ---
>>  hw/arm/smmuv3.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 42dc521..5c46102 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -560,6 +560,12 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>      }
>>  
>>      ret = smmuv3_decode_config(mr, &cfg, &event);
>> +
>> +    if (cfg.bypassed) {
>> +        ret = 0;
>> +        goto out;
>> +    }
> Thank you for spotting this bug. Yes you're correct: on bypass we
> effectively need to set the IOMMUTLBEntry perm flags.
> 
> Reading the code again I think the error handling logic is really
> confusing and if you don't mind, I suggest I submit a global fix. On
> bypass we should rather have a bypass trace event issued instead of the
> translated one. To me the aborted case is not properly handled either as
> we are going to record an event whereas we shouldn't. In case of
> abort/bypass translation structure decoding should rather return 0 I
> think instead of returning errors. Please let me know if it suits you.
> 
Ok with me

Cheers，
Jia