| Message ID | 1528378684-14487-1-git-send-email-hejianet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; > } >
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
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; }
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(+)