| Message ID | 1402549692-5224-4-git-send-email-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: > This patch adds a description of private properties for the Hisilicon > System MMU architecture. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > index f284b99..75b1351 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > @@ -15,6 +15,7 @@ conditions. > "arm,smmu-v2" > "arm,mmu-400" > "arm,mmu-500" > + "hisilicon,smmu-v1" > > depending on the particular implementation and/or the > version of the architecture implemented. > @@ -54,6 +55,21 @@ conditions. > aliases of secure registers have to be used during > SMMU configuration. > > +** Hisilicon SMMU private properties: > + > +- smmu-force-memtype : A list of StreamIDs which not translate address but > + translate attributes. The StreamIDs list here can not be > + used for map(translation) mode again. > + StreamID first, then the type list below: > + 1, cahceable, WBRAWA, Normal outer and inner write-back > + 2, non-cacheable, Normal outer and inner non-cacheable > + 3, device, nGnRE > + others, bypass > + > +- smmu-bypass-vmid : Specify which context bank is used for bypass mode. > + If omit, vmid=255 is default. If bypass and map mode can > + share a same S2CR, config vmid=0. These don't feel like they belong in the device-tree to me. Is the list of StreamIDs described in smmu-force-memtype a property of the hardware configuration, or a software policy to allow you to generate cacheable traffic for particular masters? Will
On 2014/6/17 0:39, Will Deacon wrote: > On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: >> This patch adds a description of private properties for the Hisilicon >> System MMU architecture. >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> index f284b99..75b1351 100644 >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> @@ -15,6 +15,7 @@ conditions. >> "arm,smmu-v2" >> "arm,mmu-400" >> "arm,mmu-500" >> + "hisilicon,smmu-v1" >> >> depending on the particular implementation and/or the >> version of the architecture implemented. >> @@ -54,6 +55,21 @@ conditions. >> aliases of secure registers have to be used during >> SMMU configuration. >> >> +** Hisilicon SMMU private properties: >> + >> +- smmu-force-memtype : A list of StreamIDs which not translate address but >> + translate attributes. The StreamIDs list here can not be >> + used for map(translation) mode again. >> + StreamID first, then the type list below: >> + 1, cahceable, WBRAWA, Normal outer and inner write-back >> + 2, non-cacheable, Normal outer and inner non-cacheable >> + 3, device, nGnRE >> + others, bypass >> + >> +- smmu-bypass-vmid : Specify which context bank is used for bypass mode. >> + If omit, vmid=255 is default. If bypass and map mode can >> + share a same S2CR, config vmid=0. > > These don't feel like they belong in the device-tree to me. Is the list of > StreamIDs described in smmu-force-memtype a property of the hardware > configuration, or a software policy to allow you to generate cacheable > traffic for particular masters? > > Will > > . > OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance. Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c A big issue should be discussed now. When a master use bypass mode to access memory, this means it can access all non-secure memory. So, if a master can be operated in user mode, that means a user process can access any kernel memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine output priviledge attribute. But if use bypass mode and output attribute is Unprivileged, specification have no mention about where to decide memory access is permitted or not. Do you think is a spec design bug or Implemention defined, or find some description anywhere.
> -----Original Message----- > From: linux-arm-kernel [mailto:linux-arm-kernel- > bounces@lists.infradead.org] On Behalf Of leizhen > Sent: Tuesday, June 17, 2014 1:20 PM > To: Will Deacon > Cc: huxinwei@huawei.com; Catalin Marinas; Zefan Li; linux-arm-kernel > Subject: Re: [PATCH RFC v2 3/3] documentation/iommu: Add description of > Hisilicon SMMU private binding > > On 2014/6/17 0:39, Will Deacon wrote: > > On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: > >> This patch adds a description of private properties for the Hisilicon > >> System MMU architecture. > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 > >> ++++++++++++++++ > >> 1 file changed, 16 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> index f284b99..75b1351 100644 > >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> @@ -15,6 +15,7 @@ conditions. > >> "arm,smmu-v2" > >> "arm,mmu-400" > >> "arm,mmu-500" > >> + "hisilicon,smmu-v1" > >> > >> depending on the particular implementation and/or > the > >> version of the architecture implemented. > >> @@ -54,6 +55,21 @@ conditions. > >> aliases of secure registers have to be used during > >> SMMU configuration. > >> > >> +** Hisilicon SMMU private properties: > >> + > >> +- smmu-force-memtype : A list of StreamIDs which not translate > address but > >> + translate attributes. The StreamIDs list here can > not be > >> + used for map(translation) mode again. > >> + StreamID first, then the type list below: > >> + 1, cahceable, WBRAWA, Normal outer and inner write- > back > >> + 2, non-cacheable, Normal outer and inner non- > cacheable > >> + 3, device, nGnRE > >> + others, bypass > >> + > >> +- smmu-bypass-vmid : Specify which context bank is used for bypass > mode. > >> + If omit, vmid=255 is default. If bypass and map > mode can > >> + share a same S2CR, config vmid=0. > > > > These don't feel like they belong in the device-tree to me. Is the > > list of StreamIDs described in smmu-force-memtype a property of the > > hardware configuration, or a software policy to allow you to generate > > cacheable traffic for particular masters? > > > > Will > > > > . > > > OK, I will put these description into a sperate file, hisilicon,smmu.txt > and mark a reference to arm,smmu.txt > > The latter case. Some masters driver want use cacheable(WB) attribute to > access memory, but the masters can not bring cacheable attribute. So, can > not use bypass(or transaction) mode. In fact, the master driver can use > iommu_map to create map and specify IOMMU_CACHE. But maybe the driver > does not want to map, if the memory access is very dynamically, > frequently map and unmap will decrease performance. > > Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But > now, if nobody declear need it, I will just implement it in hisi-smmu.c > > A big issue should be discussed now. When a master use bypass mode to > access memory, this means it can access all non-secure memory. So, if a > master can be operated in user mode, that means a user process can access > any kernel memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is > used for determine output priviledge attribute. But if use bypass mode > and output attribute is Unprivileged, specification have no mention about > where to decide memory access is permitted or not. Do you think is a spec > design bug or Implemention defined, or find some description anywhere. > You should be using VFIO framework for user space I/O. VFIO uses the iommu_api to setup IOMMU for user space access. This allows you to restrict user space I/O access. -Varun
On Tue, Jun 17, 2014 at 08:50:20AM +0100, leizhen wrote: > On 2014/6/17 0:39, Will Deacon wrote: > > On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: > >> This patch adds a description of private properties for the Hisilicon > >> System MMU architecture. > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ > >> 1 file changed, 16 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> index f284b99..75b1351 100644 > >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >> @@ -15,6 +15,7 @@ conditions. > >> "arm,smmu-v2" > >> "arm,mmu-400" > >> "arm,mmu-500" > >> + "hisilicon,smmu-v1" > >> > >> depending on the particular implementation and/or the > >> version of the architecture implemented. > >> @@ -54,6 +55,21 @@ conditions. > >> aliases of secure registers have to be used during > >> SMMU configuration. > >> > >> +** Hisilicon SMMU private properties: > >> + > >> +- smmu-force-memtype : A list of StreamIDs which not translate address but > >> + translate attributes. The StreamIDs list here can not be > >> + used for map(translation) mode again. > >> + StreamID first, then the type list below: > >> + 1, cahceable, WBRAWA, Normal outer and inner write-back > >> + 2, non-cacheable, Normal outer and inner non-cacheable > >> + 3, device, nGnRE > >> + others, bypass > >> + > >> +- smmu-bypass-vmid : Specify which context bank is used for bypass mode. > >> + If omit, vmid=255 is default. If bypass and map mode can > >> + share a same S2CR, config vmid=0. > > > > These don't feel like they belong in the device-tree to me. Is the list of > > StreamIDs described in smmu-force-memtype a property of the hardware > > configuration, or a software policy to allow you to generate cacheable > > traffic for particular masters? > > > > Will > > > > . > > > OK, I will put these description into a sperate file, hisilicon,smmu.txt > and mark a reference to arm,smmu.txt No, don't do that. Delete the properties instead. I'm not a device-tree expert, but these don't feel like things we should be describing there at all. > The latter case. Some masters driver want use cacheable(WB) attribute to access > memory, but the masters can not bring cacheable attribute. So, can not use > bypass(or transaction) mode. In fact, the master driver can use iommu_map to > create map and specify IOMMU_CACHE. But maybe the driver does not want to > map, if the memory access is very dynamically, frequently map and unmap will > decrease performance. You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in Linux? > Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, > if nobody declear need it, I will just implement it in hisi-smmu.c I don't want to see hisi-smmu.c at all. You need to make your driver fit into the code we already have. Do you have a specification available describing the hardware you have created? > A big issue should be discussed now. When a master use bypass mode to access > memory, this means it can access all non-secure memory. So, if a master can > be operated in user mode, that means a user process can access any kernel > memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine > output priviledge attribute. But if use bypass mode and output attribute is > Unprivileged, specification have no mention about where to decide memory access > is permitted or not. Do you think is a spec design bug or Implemention > defined, or find some description anywhere. There are various overrides for the privilege controls but, as Varun said, you should be using VFIO for userspace device access. Will
On 2014/6/17 17:33, Will Deacon wrote: > On Tue, Jun 17, 2014 at 08:50:20AM +0100, leizhen wrote: >> On 2014/6/17 0:39, Will Deacon wrote: >>> On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: >>>> This patch adds a description of private properties for the Hisilicon >>>> System MMU architecture. >>>> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>> --- >>>> Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ >>>> 1 file changed, 16 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>> index f284b99..75b1351 100644 >>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>> @@ -15,6 +15,7 @@ conditions. >>>> "arm,smmu-v2" >>>> "arm,mmu-400" >>>> "arm,mmu-500" >>>> + "hisilicon,smmu-v1" >>>> >>>> depending on the particular implementation and/or the >>>> version of the architecture implemented. >>>> @@ -54,6 +55,21 @@ conditions. >>>> aliases of secure registers have to be used during >>>> SMMU configuration. >>>> >>>> +** Hisilicon SMMU private properties: >>>> + >>>> +- smmu-force-memtype : A list of StreamIDs which not translate address but >>>> + translate attributes. The StreamIDs list here can not be >>>> + used for map(translation) mode again. >>>> + StreamID first, then the type list below: >>>> + 1, cahceable, WBRAWA, Normal outer and inner write-back >>>> + 2, non-cacheable, Normal outer and inner non-cacheable >>>> + 3, device, nGnRE >>>> + others, bypass >>>> + >>>> +- smmu-bypass-vmid : Specify which context bank is used for bypass mode. >>>> + If omit, vmid=255 is default. If bypass and map mode can >>>> + share a same S2CR, config vmid=0. >>> >>> These don't feel like they belong in the device-tree to me. Is the list of >>> StreamIDs described in smmu-force-memtype a property of the hardware >>> configuration, or a software policy to allow you to generate cacheable >>> traffic for particular masters? >>> >>> Will >>> >>> . >>> >> OK, I will put these description into a sperate file, hisilicon,smmu.txt >> and mark a reference to arm,smmu.txt > > No, don't do that. Delete the properties instead. I'm not a device-tree > expert, but these don't feel like things we should be describing there at > all. > >> The latter case. Some masters driver want use cacheable(WB) attribute to access >> memory, but the masters can not bring cacheable attribute. So, can not use >> bypass(or transaction) mode. In fact, the master driver can use iommu_map to >> create map and specify IOMMU_CACHE. But maybe the driver does not want to >> map, if the memory access is very dynamically, frequently map and unmap will >> decrease performance. > > You seem to be highlighting a perceived deficiency in the IOMMU API which > you're attempting to work-around with new device-tree properties. Instead, > why not propose an extension to the IOMMU API in Linux? > The private properties or new IOMMU APIs, just in order to optimize performance or simplify master driver code, I will consider it later. I think it's good to support base functions first. Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others. Add a common API may meet more difficulty. If I have time, I will try to do it. >> Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, >> if nobody declear need it, I will just implement it in hisi-smmu.c > > I don't want to see hisi-smmu.c at all. You need to make your driver fit > into the code we already have. > > Do you have a specification available describing the hardware you have > created? > Although I have the Hisilicon SMMU specification, but it was written in Chinese. I have told this to hardware engineers. OK. I will try my best to merge two drivers into one file. Maybe need to use many #if#else. >> A big issue should be discussed now. When a master use bypass mode to access >> memory, this means it can access all non-secure memory. So, if a master can >> be operated in user mode, that means a user process can access any kernel >> memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine >> output priviledge attribute. But if use bypass mode and output attribute is >> Unprivileged, specification have no mention about where to decide memory access >> is permitted or not. Do you think is a spec design bug or Implemention >> defined, or find some description anywhere. > > There are various overrides for the privilege controls but, as Varun said, > you should be using VFIO for userspace device access. > > Will > > . >
Hi Will/Zhen, > -----Original Message----- > From: linux-arm-kernel [mailto:linux-arm-kernel- > bounces@lists.infradead.org] On Behalf Of leizhen > Sent: Wednesday, June 18, 2014 6:59 AM > To: Will Deacon > Cc: huxinwei@huawei.com; Catalin Marinas; Zefan Li; linux-arm-kernel > Subject: Re: [PATCH RFC v2 3/3] documentation/iommu: Add description of > Hisilicon SMMU private binding > > On 2014/6/17 17:33, Will Deacon wrote: > > On Tue, Jun 17, 2014 at 08:50:20AM +0100, leizhen wrote: > >> On 2014/6/17 0:39, Will Deacon wrote: > >>> On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: > >>>> This patch adds a description of private properties for the > >>>> Hisilicon System MMU architecture. > >>>> > >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >>>> --- > >>>> Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 > >>>> ++++++++++++++++ > >>>> 1 file changed, 16 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >>>> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >>>> index f284b99..75b1351 100644 > >>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > >>>> @@ -15,6 +15,7 @@ conditions. > >>>> "arm,smmu-v2" > >>>> "arm,mmu-400" > >>>> "arm,mmu-500" > >>>> + "hisilicon,smmu-v1" > >>>> > >>>> depending on the particular implementation and/or > the > >>>> version of the architecture implemented. > >>>> @@ -54,6 +55,21 @@ conditions. > >>>> aliases of secure registers have to be used > during > >>>> SMMU configuration. > >>>> > >>>> +** Hisilicon SMMU private properties: > >>>> + > >>>> +- smmu-force-memtype : A list of StreamIDs which not translate > address but > >>>> + translate attributes. The StreamIDs list here can > not be > >>>> + used for map(translation) mode again. > >>>> + StreamID first, then the type list below: > >>>> + 1, cahceable, WBRAWA, Normal outer and inner > write-back > >>>> + 2, non-cacheable, Normal outer and inner non- > cacheable > >>>> + 3, device, nGnRE > >>>> + others, bypass > >>>> + > >>>> +- smmu-bypass-vmid : Specify which context bank is used for > bypass mode. > >>>> + If omit, vmid=255 is default. If bypass and map > mode can > >>>> + share a same S2CR, config vmid=0. > >>> > >>> These don't feel like they belong in the device-tree to me. Is the > >>> list of StreamIDs described in smmu-force-memtype a property of the > >>> hardware configuration, or a software policy to allow you to > >>> generate cacheable traffic for particular masters? > >>> > >>> Will > >>> > >>> . > >>> > >> OK, I will put these description into a sperate file, > >> hisilicon,smmu.txt and mark a reference to arm,smmu.txt > > > > No, don't do that. Delete the properties instead. I'm not a > > device-tree expert, but these don't feel like things we should be > > describing there at all. > > > >> The latter case. Some masters driver want use cacheable(WB) attribute > >> to access memory, but the masters can not bring cacheable attribute. > >> So, can not use bypass(or transaction) mode. In fact, the master > >> driver can use iommu_map to create map and specify IOMMU_CACHE. But > >> maybe the driver does not want to map, if the memory access is very > >> dynamically, frequently map and unmap will decrease performance. > > > > You seem to be highlighting a perceived deficiency in the IOMMU API > > which you're attempting to work-around with new device-tree > > properties. Instead, why not propose an extension to the IOMMU API in > Linux? > > > > The private properties or new IOMMU APIs, just in order to optimize > performance or simplify master driver code, I will consider it later. I > think it's good to support base functions first. > > Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with > others. > Add a common API may meet more difficulty. > > If I have time, I will try to do it. > [Sethi Varun-B16395] We can introduce SMMU specific attributes (to use with iommu set attribute API call) for setting memory cacheability attributes. These could be used for setting the MAIR per context bank. Actually, we would require this for out platform. However, I am not sure how memory attribute translation would work if S2CR is set for bypass? -Varun
On 2014/6/18 20:03, Varun Sethi wrote: > Hi Will/Zhen, > >> -----Original Message----- >> From: linux-arm-kernel [mailto:linux-arm-kernel- >> bounces@lists.infradead.org] On Behalf Of leizhen >> Sent: Wednesday, June 18, 2014 6:59 AM >> To: Will Deacon >> Cc: huxinwei@huawei.com; Catalin Marinas; Zefan Li; linux-arm-kernel >> Subject: Re: [PATCH RFC v2 3/3] documentation/iommu: Add description of >> Hisilicon SMMU private binding >> >> On 2014/6/17 17:33, Will Deacon wrote: >>> On Tue, Jun 17, 2014 at 08:50:20AM +0100, leizhen wrote: >>>> On 2014/6/17 0:39, Will Deacon wrote: >>>>> On Thu, Jun 12, 2014 at 06:08:12AM +0100, Zhen Lei wrote: >>>>>> This patch adds a description of private properties for the >>>>>> Hisilicon System MMU architecture. >>>>>> >>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>>>> --- >>>>>> Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 >>>>>> ++++++++++++++++ >>>>>> 1 file changed, 16 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> index f284b99..75b1351 100644 >>>>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> @@ -15,6 +15,7 @@ conditions. >>>>>> "arm,smmu-v2" >>>>>> "arm,mmu-400" >>>>>> "arm,mmu-500" >>>>>> + "hisilicon,smmu-v1" >>>>>> >>>>>> depending on the particular implementation and/or >> the >>>>>> version of the architecture implemented. >>>>>> @@ -54,6 +55,21 @@ conditions. >>>>>> aliases of secure registers have to be used >> during >>>>>> SMMU configuration. >>>>>> >>>>>> +** Hisilicon SMMU private properties: >>>>>> + >>>>>> +- smmu-force-memtype : A list of StreamIDs which not translate >> address but >>>>>> + translate attributes. The StreamIDs list here can >> not be >>>>>> + used for map(translation) mode again. >>>>>> + StreamID first, then the type list below: >>>>>> + 1, cahceable, WBRAWA, Normal outer and inner >> write-back >>>>>> + 2, non-cacheable, Normal outer and inner non- >> cacheable >>>>>> + 3, device, nGnRE >>>>>> + others, bypass >>>>>> + >>>>>> +- smmu-bypass-vmid : Specify which context bank is used for >> bypass mode. >>>>>> + If omit, vmid=255 is default. If bypass and map >> mode can >>>>>> + share a same S2CR, config vmid=0. >>>>> >>>>> These don't feel like they belong in the device-tree to me. Is the >>>>> list of StreamIDs described in smmu-force-memtype a property of the >>>>> hardware configuration, or a software policy to allow you to >>>>> generate cacheable traffic for particular masters? >>>>> >>>>> Will >>>>> >>>>> . >>>>> >>>> OK, I will put these description into a sperate file, >>>> hisilicon,smmu.txt and mark a reference to arm,smmu.txt >>> >>> No, don't do that. Delete the properties instead. I'm not a >>> device-tree expert, but these don't feel like things we should be >>> describing there at all. >>> >>>> The latter case. Some masters driver want use cacheable(WB) attribute >>>> to access memory, but the masters can not bring cacheable attribute. >>>> So, can not use bypass(or transaction) mode. In fact, the master >>>> driver can use iommu_map to create map and specify IOMMU_CACHE. But >>>> maybe the driver does not want to map, if the memory access is very >>>> dynamically, frequently map and unmap will decrease performance. >>> >>> You seem to be highlighting a perceived deficiency in the IOMMU API >>> which you're attempting to work-around with new device-tree >>> properties. Instead, why not propose an extension to the IOMMU API in >> Linux? >>> >> >> The private properties or new IOMMU APIs, just in order to optimize >> performance or simplify master driver code, I will consider it later. I >> think it's good to support base functions first. >> >> Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with >> others. >> Add a common API may meet more difficulty. >> >> If I have time, I will try to do it. >> > [Sethi Varun-B16395] We can introduce SMMU specific attributes (to use with iommu set attribute API call) for setting memory cacheability attributes. These could be used for setting the MAIR per context bank. Actually, we would require this for out platform. However, I am not sure how memory attribute translation would work if S2CR is set for bypass? > > -Varun > > > > . > The ARM SMMU v2 specification describes that: If the transaction is successfully matched in the Stream mapping table, SMMU_S2CRn determines the initial context. If SMMU_S2CRn specifies Bypass mode: • The address is not translated, meaning that the output address is identical to the input address. • Output attributes are a function of the input attributes and the SMMU_S2CRn fields.
On Wed, Jun 18, 2014 at 02:28:30AM +0100, leizhen wrote: > On 2014/6/17 17:33, Will Deacon wrote: > > On Tue, Jun 17, 2014 at 08:50:20AM +0100, leizhen wrote: > >> The latter case. Some masters driver want use cacheable(WB) attribute to access > >> memory, but the masters can not bring cacheable attribute. So, can not use > >> bypass(or transaction) mode. In fact, the master driver can use iommu_map to > >> create map and specify IOMMU_CACHE. But maybe the driver does not want to > >> map, if the memory access is very dynamically, frequently map and unmap will > >> decrease performance. > > > > You seem to be highlighting a perceived deficiency in the IOMMU API which > > you're attempting to work-around with new device-tree properties. Instead, > > why not propose an extension to the IOMMU API in Linux? > > > > The private properties or new IOMMU APIs, just in order to optimize performance > or simplify master driver code, I will consider it later. I think it's good to > support base functions first. > > Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others. > Add a common API may meet more difficulty. > > If I have time, I will try to do it. Ok, so we're agreeing to drop those properties for now? > >> Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, > >> if nobody declear need it, I will just implement it in hisi-smmu.c > > > > I don't want to see hisi-smmu.c at all. You need to make your driver fit > > into the code we already have. > > > > Do you have a specification available describing the hardware you have > > created? > > > > Although I have the Hisilicon SMMU specification, but it was written in Chinese. > I have told this to hardware engineers. Having access to a specification I can understand would help to assess exactly what you've gone and built. > OK. I will try my best to merge two drivers into one file. Maybe need to use many > #if#else. The fact of the matter is that you've got a device that isn't architecturally compliant. That leaves me in a fairly undesirable position with a small set of options: (1) We can duplicate lots of the code we already have and you end up with a separate driver. This is obviously bad because of the code duplication, associated maintenance headaches, driver divergence, etc. It also brings into question the point of having a driver written to the architecture when the hardware has gone off and done something different. (2) We try to fit your SMMU into the existing driver. I'd like to see what this looks like, but if it's as much #ifdeffery as you suggest, then that's also bad for many of the same reasons as above. (3) We don't support your device in the Linux kernel. We could just treat your SMMU as a broken ARM SMMU implementation and not support it. If this was a CPU instead of an SMMU this would unquestionably be the correct approach. Unless you can come up with something compelling for point (2), I'm going to err on the side of (3). So, please, *please*, don't do a rushed job of merging the drivers. You need to convince me why I should bother supporting this device in my driver, which means using clean abstractions and extending the code we already have. If this isn't possible, then perhaps you'll consider following the architecture next time around. Will
On 2014/6/18 21:32, Will Deacon wrote: > On Wed, Jun 18, 2014 at 02:28:30AM +0100, leizhen wrote: >> On 2014/6/17 17:33, Will Deacon wrote: >>> On Tue, Jun 17, 2014 at 08:50:20AM +0100, leizhen wrote: >>>> The latter case. Some masters driver want use cacheable(WB) attribute to access >>>> memory, but the masters can not bring cacheable attribute. So, can not use >>>> bypass(or transaction) mode. In fact, the master driver can use iommu_map to >>>> create map and specify IOMMU_CACHE. But maybe the driver does not want to >>>> map, if the memory access is very dynamically, frequently map and unmap will >>>> decrease performance. >>> >>> You seem to be highlighting a perceived deficiency in the IOMMU API which >>> you're attempting to work-around with new device-tree properties. Instead, >>> why not propose an extension to the IOMMU API in Linux? >>> >> >> The private properties or new IOMMU APIs, just in order to optimize performance >> or simplify master driver code, I will consider it later. I think it's good to >> support base functions first. >> >> Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others. >> Add a common API may meet more difficulty. >> >> If I have time, I will try to do it. > > Ok, so we're agreeing to drop those properties for now? > >>>> Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, >>>> if nobody declear need it, I will just implement it in hisi-smmu.c >>> >>> I don't want to see hisi-smmu.c at all. You need to make your driver fit >>> into the code we already have. >>> >>> Do you have a specification available describing the hardware you have >>> created? >>> >> >> Although I have the Hisilicon SMMU specification, but it was written in Chinese. >> I have told this to hardware engineers. > > Having access to a specification I can understand would help to assess > exactly what you've gone and built. > >> OK. I will try my best to merge two drivers into one file. Maybe need to use many >> #if#else. > > The fact of the matter is that you've got a device that isn't > architecturally compliant. That leaves me in a fairly undesirable position > with a small set of options: > > (1) We can duplicate lots of the code we already have and you end up with a > separate driver. This is obviously bad because of the code duplication, > associated maintenance headaches, driver divergence, etc. It also brings > into question the point of having a driver written to the architecture > when the hardware has gone off and done something different. > > (2) We try to fit your SMMU into the existing driver. I'd like to see what > this looks like, but if it's as much #ifdeffery as you suggest, then > that's also bad for many of the same reasons as above. > > (3) We don't support your device in the Linux kernel. We could just treat > your SMMU as a broken ARM SMMU implementation and not support it. If > this was a CPU instead of an SMMU this would unquestionably be the > correct approach. > > Unless you can come up with something compelling for point (2), I'm going to > err on the side of (3). So, please, *please*, don't do a rushed job of > merging the drivers. You need to convince me why I should bother supporting > this device in my driver, which means using clean abstractions and extending > the code we already have. If this isn't possible, then perhaps you'll > consider following the architecture next time around. > > Will > > . > Yeah, I konw. I also a software engineer. A victim of this SMMU.
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt index f284b99..75b1351 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt @@ -15,6 +15,7 @@ conditions. "arm,smmu-v2" "arm,mmu-400" "arm,mmu-500" + "hisilicon,smmu-v1" depending on the particular implementation and/or the version of the architecture implemented. @@ -54,6 +55,21 @@ conditions. aliases of secure registers have to be used during SMMU configuration. +** Hisilicon SMMU private properties: + +- smmu-force-memtype : A list of StreamIDs which not translate address but + translate attributes. The StreamIDs list here can not be + used for map(translation) mode again. + StreamID first, then the type list below: + 1, cahceable, WBRAWA, Normal outer and inner write-back + 2, non-cacheable, Normal outer and inner non-cacheable + 3, device, nGnRE + others, bypass + +- smmu-bypass-vmid : Specify which context bank is used for bypass mode. + If omit, vmid=255 is default. If bypass and map mode can + share a same S2CR, config vmid=0. + Example: smmu {
This patch adds a description of private properties for the Hisilicon System MMU architecture. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) -- 1.8.0