Re: [PATCH] VFIO: Fix Documentation

2018-05-06 Thread dongbo (E) 
Hi, Alex.

On 2018/5/1 2:38, Alex Williamson wrote:
> On Fri, 20 Apr 2018 18:07:27 +0800
> "dongbo (E)" <dong...@huawei.com> wrote:
> 
>> From: Dong Bo <dong...@huawei.com>
>>
>> Signed-off-by: Dong Bo <dong...@huawei.com>
>> ---
> 
> Hi Dong Bo,
> 
> The patch is corrupted, please resend and also include a commit log,
> something as simple as "Update vfio_add_group_dev description to match
> the current API" would be fine.  Thanks,
> 
> Alex
> 
Sorry for the corrupted patch. I've resent one and added the commit log
you suggested. Hope it would apply. Thanks.

Best Regards,
Dong Bo

>>  Documentation/vfio.txt | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> index ef6a511..f1a4d3c 100644
>> --- a/Documentation/vfio.txt
>> +++ b/Documentation/vfio.txt
>> @@ -252,15 +252,14 @@ into VFIO core.  When devices are bound and unbound to 
>> the driver,
>>  the driver should call vfio_add_group_dev() and vfio_del_group_dev()
>>  respectively::
>>  -   extern int vfio_add_group_dev(struct iommu_group *iommu_group,
>> -  struct device *dev,
>> +extern int vfio_add_group_dev(struct device *dev,
>>const struct vfio_device_ops *ops,
>>void *device_data);
>>  extern void *vfio_del_group_dev(struct device *dev);
>>   vfio_add_group_dev() indicates to the core to begin tracking the
>> -specified iommu_group and register the specified dev as owned by
>> +iommu_group of the specified dev and register the dev as owned by
>>  a VFIO bus driver.  The driver provides an ops structure for callbacks
>>  similar to a file operations structure::
>>  -- 1.9.1
>>
>>
>> .
>>
>>
> 
> 
> .
>

Re: [PATCH] VFIO: Fix Documentation

2018-05-06 Thread dongbo (E) 
Hi, Alex.

On 2018/5/1 2:38, Alex Williamson wrote:
> On Fri, 20 Apr 2018 18:07:27 +0800
> "dongbo (E)"  wrote:
> 
>> From: Dong Bo 
>>
>> Signed-off-by: Dong Bo 
>> ---
> 
> Hi Dong Bo,
> 
> The patch is corrupted, please resend and also include a commit log,
> something as simple as "Update vfio_add_group_dev description to match
> the current API" would be fine.  Thanks,
> 
> Alex
> 
Sorry for the corrupted patch. I've resent one and added the commit log
you suggested. Hope it would apply. Thanks.

Best Regards,
Dong Bo

>>  Documentation/vfio.txt | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> index ef6a511..f1a4d3c 100644
>> --- a/Documentation/vfio.txt
>> +++ b/Documentation/vfio.txt
>> @@ -252,15 +252,14 @@ into VFIO core.  When devices are bound and unbound to 
>> the driver,
>>  the driver should call vfio_add_group_dev() and vfio_del_group_dev()
>>  respectively::
>>  -   extern int vfio_add_group_dev(struct iommu_group *iommu_group,
>> -  struct device *dev,
>> +extern int vfio_add_group_dev(struct device *dev,
>>const struct vfio_device_ops *ops,
>>void *device_data);
>>  extern void *vfio_del_group_dev(struct device *dev);
>>   vfio_add_group_dev() indicates to the core to begin tracking the
>> -specified iommu_group and register the specified dev as owned by
>> +iommu_group of the specified dev and register the dev as owned by
>>  a VFIO bus driver.  The driver provides an ops structure for callbacks
>>  similar to a file operations structure::
>>  -- 1.9.1
>>
>>
>> .
>>
>>
> 
> 
> .
>

[PATCH] vfio: fix documentation

2018-05-06 Thread dongbo (E) 
From: Dong Bo 

Update vfio_add_group_dev description to match the current API.

Signed-off-by: Dong Bo 
---
 Documentation/vfio.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index ef6a511..f1a4d3c 100644
--- a/Documentation/vfio.txt
+++ b/Documentation/vfio.txt
@@ -252,15 +252,14 @@ into VFIO core.  When devices are bound and unbound to 
the driver,
 the driver should call vfio_add_group_dev() and vfio_del_group_dev()
 respectively::

-   extern int vfio_add_group_dev(struct iommu_group *iommu_group,
- struct device *dev,
+   extern int vfio_add_group_dev(struct device *dev,
  const struct vfio_device_ops *ops,
  void *device_data);

extern void *vfio_del_group_dev(struct device *dev);

 vfio_add_group_dev() indicates to the core to begin tracking the
-specified iommu_group and register the specified dev as owned by
+iommu_group of the specified dev and register the dev as owned by
 a VFIO bus driver.  The driver provides an ops structure for callbacks
 similar to a file operations structure::

-- 
1.9.1


.

[PATCH] vfio: fix documentation

2018-05-06 Thread dongbo (E) 
From: Dong Bo 

Update vfio_add_group_dev description to match the current API.

Signed-off-by: Dong Bo 
---
 Documentation/vfio.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index ef6a511..f1a4d3c 100644
--- a/Documentation/vfio.txt
+++ b/Documentation/vfio.txt
@@ -252,15 +252,14 @@ into VFIO core.  When devices are bound and unbound to 
the driver,
 the driver should call vfio_add_group_dev() and vfio_del_group_dev()
 respectively::

-   extern int vfio_add_group_dev(struct iommu_group *iommu_group,
- struct device *dev,
+   extern int vfio_add_group_dev(struct device *dev,
  const struct vfio_device_ops *ops,
  void *device_data);

extern void *vfio_del_group_dev(struct device *dev);

 vfio_add_group_dev() indicates to the core to begin tracking the
-specified iommu_group and register the specified dev as owned by
+iommu_group of the specified dev and register the dev as owned by
 a VFIO bus driver.  The driver provides an ops structure for callbacks
 similar to a file operations structure::

-- 
1.9.1


.

[PATCH] VFIO: Fix Documentation

2018-04-20 Thread dongbo (E) 
From: Dong Bo 

Signed-off-by: Dong Bo 
---
 Documentation/vfio.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index ef6a511..f1a4d3c 100644
--- a/Documentation/vfio.txt
+++ b/Documentation/vfio.txt
@@ -252,15 +252,14 @@ into VFIO core.  When devices are bound and unbound to 
the driver,
 the driver should call vfio_add_group_dev() and vfio_del_group_dev()
 respectively::
 -  extern int vfio_add_group_dev(struct iommu_group *iommu_group,
- struct device *dev,
+   extern int vfio_add_group_dev(struct device *dev,
  const struct vfio_device_ops *ops,
  void *device_data);
extern void *vfio_del_group_dev(struct device *dev);
  vfio_add_group_dev() indicates to the core to begin tracking the
-specified iommu_group and register the specified dev as owned by
+iommu_group of the specified dev and register the dev as owned by
 a VFIO bus driver.  The driver provides an ops structure for callbacks
 similar to a file operations structure::
 -- 1.9.1


.

[PATCH] VFIO: Fix Documentation

2018-04-20 Thread dongbo (E) 
From: Dong Bo 

Signed-off-by: Dong Bo 
---
 Documentation/vfio.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index ef6a511..f1a4d3c 100644
--- a/Documentation/vfio.txt
+++ b/Documentation/vfio.txt
@@ -252,15 +252,14 @@ into VFIO core.  When devices are bound and unbound to 
the driver,
 the driver should call vfio_add_group_dev() and vfio_del_group_dev()
 respectively::
 -  extern int vfio_add_group_dev(struct iommu_group *iommu_group,
- struct device *dev,
+   extern int vfio_add_group_dev(struct device *dev,
  const struct vfio_device_ops *ops,
  void *device_data);
extern void *vfio_del_group_dev(struct device *dev);
  vfio_add_group_dev() indicates to the core to begin tracking the
-specified iommu_group and register the specified dev as owned by
+iommu_group of the specified dev and register the dev as owned by
 a VFIO bus driver.  The driver provides an ops structure for callbacks
 similar to a file operations structure::
 -- 1.9.1


.

[PATCH] Fix compile warning with ATA_DEBUG enabled

2018-01-25 Thread dongbo (E) 
From: Dong Bo 

This fixs the following comile warnings with ATA_DEBUG enabled,
which detected by Linaro GCC 5.2-2015.11:

In file included from ./include/linux/printk.h:7:0,
 from ./include/linux/kernel.h:14,
 from ./include/asm-generic/bug.h:16,
 from ./arch/arm64/include/asm/bug.h:37,
 from ./include/linux/bug.h:5,
 from ./include/linux/mmdebug.h:5,
 from ./include/linux/gfp.h:5,
 from ./include/linux/slab.h:15,
 from drivers/ata/libata-scsi.c:36:
drivers/ata/libata-scsi.c: In function 'ata_scsi_dump_cdb':
./include/linux/kern_levels.h:5:18: warning: format '%d' expects
argument of type 'int', but argument 6 has type 'u64 {aka long
 long unsigned int}' [-Wformat=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
  ^
./include/linux/kern_levels.h:11:18: note: in expansion of macro
'KERN_SOH'
 #define KERN_ERR KERN_SOH "3" /* error conditions */
  ^
./include/linux/libata.h:65:38: note: in expansion of macro 'KERN_ERR'
 #define DPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt,
__func__, ## args)
  ^
drivers/ata/libata-scsi.c:4284:2: note: in expansion of macro 'DPRINTK'
  DPRINTK("CDB (%u:%d,%d,%d) %9ph\n",
  ^

Signed-off-by: Dong Bo 
---
 drivers/ata/libata-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 66be961..214ecb6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4282,7 +4282,7 @@ static inline void ata_scsi_dump_cdb(struct ata_port *ap,
 #ifdef ATA_DEBUG
struct scsi_device *scsidev = cmd->device;
 -  DPRINTK("CDB (%u:%d,%d,%d) %9ph\n",
+   DPRINTK("CDB (%u:%u,%u,%lld) %9ph\n",
ap->print_id,
scsidev->channel, scsidev->id, scsidev->lun,
cmd->cmnd);
-- 
1.9.1


.

[PATCH] Fix compile warning with ATA_DEBUG enabled

2018-01-25 Thread dongbo (E) 
From: Dong Bo 

This fixs the following comile warnings with ATA_DEBUG enabled,
which detected by Linaro GCC 5.2-2015.11:

In file included from ./include/linux/printk.h:7:0,
 from ./include/linux/kernel.h:14,
 from ./include/asm-generic/bug.h:16,
 from ./arch/arm64/include/asm/bug.h:37,
 from ./include/linux/bug.h:5,
 from ./include/linux/mmdebug.h:5,
 from ./include/linux/gfp.h:5,
 from ./include/linux/slab.h:15,
 from drivers/ata/libata-scsi.c:36:
drivers/ata/libata-scsi.c: In function 'ata_scsi_dump_cdb':
./include/linux/kern_levels.h:5:18: warning: format '%d' expects
argument of type 'int', but argument 6 has type 'u64 {aka long
 long unsigned int}' [-Wformat=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
  ^
./include/linux/kern_levels.h:11:18: note: in expansion of macro
'KERN_SOH'
 #define KERN_ERR KERN_SOH "3" /* error conditions */
  ^
./include/linux/libata.h:65:38: note: in expansion of macro 'KERN_ERR'
 #define DPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt,
__func__, ## args)
  ^
drivers/ata/libata-scsi.c:4284:2: note: in expansion of macro 'DPRINTK'
  DPRINTK("CDB (%u:%d,%d,%d) %9ph\n",
  ^

Signed-off-by: Dong Bo 
---
 drivers/ata/libata-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 66be961..214ecb6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4282,7 +4282,7 @@ static inline void ata_scsi_dump_cdb(struct ata_port *ap,
 #ifdef ATA_DEBUG
struct scsi_device *scsidev = cmd->device;
 -  DPRINTK("CDB (%u:%d,%d,%d) %9ph\n",
+   DPRINTK("CDB (%u:%u,%u,%lld) %9ph\n",
ap->print_id,
scsidev->channel, scsidev->id, scsidev->lun,
cmd->cmnd);
-- 
1.9.1


.

Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-25 Thread dongbo (E) 


On 2017/4/24 23:58, Catalin Marinas wrote:
> On Mon, Apr 24, 2017 at 04:40:23PM +0100, Will Deacon wrote:
>> On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote:
>>> On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote:
>>>> On 18 April 2017 at 18:01, Catalin Marinas <catalin.mari...@arm.com> wrote:
>>>>> On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote:
>>>>>> From: Dong Bo <dong...@huawei.com>
>>>>>>
>>>>>> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
>>>>>> the flag is propagated to its child processes, even the elf
>>>>>> files are marked as not requiring executable stack. It may
>>>>>> cause superfluous operations on some arch, e.g.
>>>>>> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is
>>>>>> also marked as PROT_EXEC.
>>>>
>>>>> That's affecting most architectures with a risk of ABI breakage. We
>>>>> could do it on arm64 only, though I'm not yet clear on the ABI
>>>>> implications (at a first look, there shouldn't be any).
>>>>
>>>> Is there a reason why it isn't just straightforwardly a bug
>>>> (which we could fix) to make READ_IMPLIES_EXEC propagate to
>>>> child processes?
>>>
>>> While I agree that it looks like a bug, if there are user programs
>>> relying on such bug we call it "ABI". On arm64, I don't think there is
>>> anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change
>>> the compat task handling without the corresponding change in arch/arm.
>>>
>>>> AFAICT this should be per-process: just because
>>>> init happens not to have been (re)compiled to permit non-executable
>>>> stacks doesn't mean every process on the system needs to have
>>>> an executable stack.
>>>
>>> I think this also affects the heap if brk(2) is used (via
>>> VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these
>>> days).
>>
>> I think it also affects mprotect, which is more worrying imo, particularly
>> for things like JIT code that is ported from 32-bit (although a quick look
>> at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed).
> 
> As Peter said, the default behaviour is READ_IMPLIES_EXEC off, so JIT
> code must already pass PROT_EXEC if it wants executable permission. The
> question is whether any user code relies on READ_IMPLIES_EXEC being
> passed down to child processes. I don't think so but I would be
> reluctant to make an such cross-arch change (happy to do it for arm64
> though).
> 

OK, I have re-built a patch for arm64 as you suggested. Thanks.

> Since linux-arch was cc'ed in the middle of this thread, I doubt people
> would reply. I suggest that the original patch is re-posted to
> linux-arch directly.
> 
Re-posted.

Bo Dong
.

Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-25 Thread dongbo (E) 


On 2017/4/24 23:58, Catalin Marinas wrote:
> On Mon, Apr 24, 2017 at 04:40:23PM +0100, Will Deacon wrote:
>> On Wed, Apr 19, 2017 at 11:33:14AM +0100, Catalin Marinas wrote:
>>> On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote:
>>>> On 18 April 2017 at 18:01, Catalin Marinas  wrote:
>>>>> On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote:
>>>>>> From: Dong Bo 
>>>>>>
>>>>>> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
>>>>>> the flag is propagated to its child processes, even the elf
>>>>>> files are marked as not requiring executable stack. It may
>>>>>> cause superfluous operations on some arch, e.g.
>>>>>> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is
>>>>>> also marked as PROT_EXEC.
>>>>
>>>>> That's affecting most architectures with a risk of ABI breakage. We
>>>>> could do it on arm64 only, though I'm not yet clear on the ABI
>>>>> implications (at a first look, there shouldn't be any).
>>>>
>>>> Is there a reason why it isn't just straightforwardly a bug
>>>> (which we could fix) to make READ_IMPLIES_EXEC propagate to
>>>> child processes?
>>>
>>> While I agree that it looks like a bug, if there are user programs
>>> relying on such bug we call it "ABI". On arm64, I don't think there is
>>> anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change
>>> the compat task handling without the corresponding change in arch/arm.
>>>
>>>> AFAICT this should be per-process: just because
>>>> init happens not to have been (re)compiled to permit non-executable
>>>> stacks doesn't mean every process on the system needs to have
>>>> an executable stack.
>>>
>>> I think this also affects the heap if brk(2) is used (via
>>> VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these
>>> days).
>>
>> I think it also affects mprotect, which is more worrying imo, particularly
>> for things like JIT code that is ported from 32-bit (although a quick look
>> at v8, ionmonkey and art suggests they all pass PROT_EXEC when needed).
> 
> As Peter said, the default behaviour is READ_IMPLIES_EXEC off, so JIT
> code must already pass PROT_EXEC if it wants executable permission. The
> question is whether any user code relies on READ_IMPLIES_EXEC being
> passed down to child processes. I don't think so but I would be
> reluctant to make an such cross-arch change (happy to do it for arm64
> though).
> 

OK, I have re-built a patch for arm64 as you suggested. Thanks.

> Since linux-arch was cc'ed in the middle of this thread, I doubt people
> would reply. I suggest that the original patch is re-posted to
> linux-arch directly.
> 
Re-posted.

Bo Dong
.

[PATCH REPOST] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-25 Thread dongbo (E) 
From: Dong Bo 

In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
the flag is propagated to its child processes, even the elf
files are marked as not requiring executable stack. It may
cause superfluous operations on some arch, e.g.
__sync_icache_dcache on aarch64 due to a PROT_READ mmap is
also marked as PROT_EXEC.

This patch was originally posted and discussed here:
https://patchwork.kernel.org/patch/9685891/

Signed-off-by: Dong Bo 
---
 fs/binfmt_elf.c   | 2 ++
 fs/binfmt_elf_fdpic.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5..c52e670 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
SET_PERSONALITY2(loc->elf_ex, _state);
if (elf_read_implies_exec(loc->elf_ex, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
current->flags |= PF_RANDOMIZE;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4f..c4bc4d0 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
set_personality(PER_LINUX);
if (elf_read_implies_exec(_params.hdr, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
setup_new_exec(bprm);
 -- 1.9.1


.

[PATCH REPOST] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-25 Thread dongbo (E) 
From: Dong Bo 

In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
the flag is propagated to its child processes, even the elf
files are marked as not requiring executable stack. It may
cause superfluous operations on some arch, e.g.
__sync_icache_dcache on aarch64 due to a PROT_READ mmap is
also marked as PROT_EXEC.

This patch was originally posted and discussed here:
https://patchwork.kernel.org/patch/9685891/

Signed-off-by: Dong Bo 
---
 fs/binfmt_elf.c   | 2 ++
 fs/binfmt_elf_fdpic.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5..c52e670 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
SET_PERSONALITY2(loc->elf_ex, _state);
if (elf_read_implies_exec(loc->elf_ex, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
current->flags |= PF_RANDOMIZE;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4f..c4bc4d0 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
set_personality(PER_LINUX);
if (elf_read_implies_exec(_params.hdr, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
setup_new_exec(bprm);
 -- 1.9.1


.

[PATCH] arm64: Preventing READ_IMPLIES_EXEC propagation

2017-04-25 Thread dongbo (E) 
From: Dong Bo 

Once the READ_IMPLIES_EXEC flag is set on arm64, the flag is
propagated to its child processes, even the ELF files are
marked as not requiring executable stack.

Signed-off-by: Dong Bo 
---
 arch/arm64/include/asm/elf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 5d17004..5941e7f 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -142,6 +142,7 @@
 ({ \
clear_bit(TIF_32BIT, >mm->context.flags);  \
clear_thread_flag(TIF_32BIT);   \
+   current->personality &= ~READ_IMPLIES_EXEC; \
 })
  /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
-- 
1.9.1

.

[PATCH] arm64: Preventing READ_IMPLIES_EXEC propagation

2017-04-25 Thread dongbo (E) 
From: Dong Bo 

Once the READ_IMPLIES_EXEC flag is set on arm64, the flag is
propagated to its child processes, even the ELF files are
marked as not requiring executable stack.

Signed-off-by: Dong Bo 
---
 arch/arm64/include/asm/elf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 5d17004..5941e7f 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -142,6 +142,7 @@
 ({ \
clear_bit(TIF_32BIT, >mm->context.flags);  \
clear_thread_flag(TIF_32BIT);   \
+   current->personality &= ~READ_IMPLIES_EXEC; \
 })
  /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
-- 
1.9.1

.

Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-19 Thread dongbo (E) 
On 2017/4/19 18:33, Catalin Marinas wrote:
> On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote:
>> On 18 April 2017 at 18:01, Catalin Marinas <catalin.mari...@arm.com> wrote:
>>> On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote:
>>>> From: Dong Bo <dong...@huawei.com>
>>>>
>>>> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
>>>> the flag is propagated to its child processes, even the elf
>>>> files are marked as not requiring executable stack. It may
>>>> cause superfluous operations on some arch, e.g.
>>>> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is
>>>> also marked as PROT_EXEC.
>>>>
>>>> Signed-off-by: Dong Bo <dong...@huawei.com>
>>>> ---
>>>>  fs/binfmt_elf.c   | 2 ++
>>>>  fs/binfmt_elf_fdpic.c | 2 ++
>>>>  2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>>>> index 5075fd5..c52e670 100644
>>>> --- a/fs/binfmt_elf.c
>>>> +++ b/fs/binfmt_elf.c
>>>> @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
>>>>SET_PERSONALITY2(loc->elf_ex, _state);
>>>>if (elf_read_implies_exec(loc->elf_ex, executable_stack))
>>>>current->personality |= READ_IMPLIES_EXEC;
>>>> +  else
>>>> +  current->personality &= ~READ_IMPLIES_EXEC;
>>>>if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
>>>>current->flags |= PF_RANDOMIZE;
>>>> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
>>>> index cf93a4f..c4bc4d0 100644
>>>> --- a/fs/binfmt_elf_fdpic.c
>>>> +++ b/fs/binfmt_elf_fdpic.c
>>>> @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm 
>>>> *bprm)
>>>>set_personality(PER_LINUX);
>>>>if (elf_read_implies_exec(_params.hdr, executable_stack))
>>>>current->personality |= READ_IMPLIES_EXEC;
>>>> +  else
>>>> +  current->personality &= ~READ_IMPLIES_EXEC;
>>>>setup_new_exec(bprm);
>>
>>> That's affecting most architectures with a risk of ABI breakage. We
>>> could do it on arm64 only, though I'm not yet clear on the ABI
>>> implications (at a first look, there shouldn't be any).
>>
>> Is there a reason why it isn't just straightforwardly a bug
>> (which we could fix) to make READ_IMPLIES_EXEC propagate to
>> child processes?
> 
> While I agree that it looks like a bug, if there are user programs
> relying on such bug we call it "ABI". On arm64, I don't think there is
> anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change
> the compat task handling without the corresponding change in arch/arm.
> 

With READ_IMPLIES_EXEC propagation, several hundreds times of
__sync_icache_dcache operations shows up than not READ_IMPLIES_EXEC
propagation, which degenerating the system performance. Changing arm64
only would settle our problem down, thanks for figuring out previously.

Seems that arch/arm had discussed the propagation of READ_IMPLIES_EXEC:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/086490.html
But the READ_IMPLIES_EXEC is still not cleared in elf_set_personality().

>> AFAICT this should be per-process: just because
>> init happens not to have been (re)compiled to permit non-executable
>> stacks doesn't mean every process on the system needs to have
>> an executable stack.
> 
> I think this also affects the heap if brk(2) is used (via
> VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these
> days).
> 
>> Behaviour shouldn't be variable across architectures either, I would
>> hope.
> 
> The behaviour has already been variable for a long time. Even on x86,
> AFAICT x86_32 differs from x86_64 in this respect.
> 
> Anyway, the patch should be posted to linux-arch for a cross-arch
> discussion.
> 

OK, this mail Cc to linux-arch. Thanks.

Re: [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-19 Thread dongbo (E) 
On 2017/4/19 18:33, Catalin Marinas wrote:
> On Tue, Apr 18, 2017 at 09:01:52PM +0100, Peter Maydell wrote:
>> On 18 April 2017 at 18:01, Catalin Marinas  wrote:
>>> On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote:
>>>> From: Dong Bo 
>>>>
>>>> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
>>>> the flag is propagated to its child processes, even the elf
>>>> files are marked as not requiring executable stack. It may
>>>> cause superfluous operations on some arch, e.g.
>>>> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is
>>>> also marked as PROT_EXEC.
>>>>
>>>> Signed-off-by: Dong Bo 
>>>> ---
>>>>  fs/binfmt_elf.c   | 2 ++
>>>>  fs/binfmt_elf_fdpic.c | 2 ++
>>>>  2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>>>> index 5075fd5..c52e670 100644
>>>> --- a/fs/binfmt_elf.c
>>>> +++ b/fs/binfmt_elf.c
>>>> @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
>>>>SET_PERSONALITY2(loc->elf_ex, _state);
>>>>if (elf_read_implies_exec(loc->elf_ex, executable_stack))
>>>>current->personality |= READ_IMPLIES_EXEC;
>>>> +  else
>>>> +  current->personality &= ~READ_IMPLIES_EXEC;
>>>>if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
>>>>current->flags |= PF_RANDOMIZE;
>>>> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
>>>> index cf93a4f..c4bc4d0 100644
>>>> --- a/fs/binfmt_elf_fdpic.c
>>>> +++ b/fs/binfmt_elf_fdpic.c
>>>> @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm 
>>>> *bprm)
>>>>set_personality(PER_LINUX);
>>>>if (elf_read_implies_exec(_params.hdr, executable_stack))
>>>>current->personality |= READ_IMPLIES_EXEC;
>>>> +  else
>>>> +  current->personality &= ~READ_IMPLIES_EXEC;
>>>>setup_new_exec(bprm);
>>
>>> That's affecting most architectures with a risk of ABI breakage. We
>>> could do it on arm64 only, though I'm not yet clear on the ABI
>>> implications (at a first look, there shouldn't be any).
>>
>> Is there a reason why it isn't just straightforwardly a bug
>> (which we could fix) to make READ_IMPLIES_EXEC propagate to
>> child processes?
> 
> While I agree that it looks like a bug, if there are user programs
> relying on such bug we call it "ABI". On arm64, I don't think there is
> anything relying on inheriting READ_IMPLIES_EXEC but I wouldn't change
> the compat task handling without the corresponding change in arch/arm.
> 

With READ_IMPLIES_EXEC propagation, several hundreds times of
__sync_icache_dcache operations shows up than not READ_IMPLIES_EXEC
propagation, which degenerating the system performance. Changing arm64
only would settle our problem down, thanks for figuring out previously.

Seems that arch/arm had discussed the propagation of READ_IMPLIES_EXEC:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/086490.html
But the READ_IMPLIES_EXEC is still not cleared in elf_set_personality().

>> AFAICT this should be per-process: just because
>> init happens not to have been (re)compiled to permit non-executable
>> stacks doesn't mean every process on the system needs to have
>> an executable stack.
> 
> I think this also affects the heap if brk(2) is used (via
> VM_DATA_DEFAULT_FLAGS though I guess malloc mostly uses mmap these
> days).
> 
>> Behaviour shouldn't be variable across architectures either, I would
>> hope.
> 
> The behaviour has already been variable for a long time. Even on x86,
> AFAICT x86_32 differs from x86_64 in this respect.
> 
> Anyway, the patch should be posted to linux-arch for a cross-arch
> discussion.
> 

OK, this mail Cc to linux-arch. Thanks.

[PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-13 Thread dongbo (E) 
From: Dong Bo 

In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
the flag is propagated to its child processes, even the elf
files are marked as not requiring executable stack. It may
cause superfluous operations on some arch, e.g.
__sync_icache_dcache on aarch64 due to a PROT_READ mmap is
also marked as PROT_EXEC.

Signed-off-by: Dong Bo 
---
 fs/binfmt_elf.c   | 2 ++
 fs/binfmt_elf_fdpic.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5..c52e670 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
SET_PERSONALITY2(loc->elf_ex, _state);
if (elf_read_implies_exec(loc->elf_ex, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
current->flags |= PF_RANDOMIZE;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4f..c4bc4d0 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
set_personality(PER_LINUX);
if (elf_read_implies_exec(_params.hdr, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
setup_new_exec(bprm);
 -- 1.9.1


.

[PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation

2017-04-13 Thread dongbo (E) 
From: Dong Bo 

In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
the flag is propagated to its child processes, even the elf
files are marked as not requiring executable stack. It may
cause superfluous operations on some arch, e.g.
__sync_icache_dcache on aarch64 due to a PROT_READ mmap is
also marked as PROT_EXEC.

Signed-off-by: Dong Bo 
---
 fs/binfmt_elf.c   | 2 ++
 fs/binfmt_elf_fdpic.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5..c52e670 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
SET_PERSONALITY2(loc->elf_ex, _state);
if (elf_read_implies_exec(loc->elf_ex, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
current->flags |= PF_RANDOMIZE;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4f..c4bc4d0 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
set_personality(PER_LINUX);
if (elf_read_implies_exec(_params.hdr, executable_stack))
current->personality |= READ_IMPLIES_EXEC;
+   else
+   current->personality &= ~READ_IMPLIES_EXEC;
setup_new_exec(bprm);
 -- 1.9.1


.

Re: [PATCH] Exchange the Assignments of `MEMORYs' and `CFGs/IOs' in Designware PCIe Driver

2016-07-04 Thread dongbo (E) 
On 2016/7/3 23:27, Pratyush Anand wrote:
> On Tue, Jun 28, 2016 at 1:42 PM, dongbo (E) <dong...@huawei.com> wrote:
>>
>> From: Dong Bo <dong...@huawei.com>
>>
>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
>> When sending CFGs to peripherals (e.g. lspci), iatu0 frequently switches
>> between CFG and IO alternatively.
>>
>> A MEMORY probably be sent as an IOs by mistake. Considering the following
>> configurations:
>> MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
>> CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
>> IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io
>>
>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set
>> the BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io. When another
>> CFG comes, the BASE_ADDR is set to 0xb400 to switch to CFG. At this
>> moment, a MEMORY access shows up, since it matches with iatu0
>> (due to 0xb400 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFF), it
>> is treated as an IO access by mistake, then sent to perpheral.
>>
>> This patch fixes the problem by exchanging the assignments of `MEMORYs'
>> and `CFGs/IOs', which assigning MEMEORYs to iatu0, CFGs and IOs to iatu1.
> 
> 
> Had a re-thought on it. While it will fix wrong memory access in your
> case, it can still cause issues with IO access for some other
> platform.
> 
> Can you please test [1] and check it that works for you. You will need
> to define num-viewport in your device tree file.
> 
> ~Pratyush
> 
> [1] 
> https://github.com/pratyushanand/linux/commit/131b83ea7db0834d77ee5df65c6696bccbf8a1ce
> 
> .
> 

Checked, it works for us.

I think it would be better to exchange the assignments of MEMORYs and
CFGs/IOs when num_viewports <= 2, cause it fixes wrong memory access.

As you mentioned, other corner point for failure is still there while
there are only two viewports. It seems that there is not a perfect
solution.

Best Regards
Dong Bo

Re: [PATCH] Exchange the Assignments of `MEMORYs' and `CFGs/IOs' in Designware PCIe Driver

2016-07-04 Thread dongbo (E) 
On 2016/7/3 23:27, Pratyush Anand wrote:
> On Tue, Jun 28, 2016 at 1:42 PM, dongbo (E)  wrote:
>>
>> From: Dong Bo 
>>
>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
>> When sending CFGs to peripherals (e.g. lspci), iatu0 frequently switches
>> between CFG and IO alternatively.
>>
>> A MEMORY probably be sent as an IOs by mistake. Considering the following
>> configurations:
>> MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
>> CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
>> IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io
>>
>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set
>> the BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io. When another
>> CFG comes, the BASE_ADDR is set to 0xb400 to switch to CFG. At this
>> moment, a MEMORY access shows up, since it matches with iatu0
>> (due to 0xb400 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFF), it
>> is treated as an IO access by mistake, then sent to perpheral.
>>
>> This patch fixes the problem by exchanging the assignments of `MEMORYs'
>> and `CFGs/IOs', which assigning MEMEORYs to iatu0, CFGs and IOs to iatu1.
> 
> 
> Had a re-thought on it. While it will fix wrong memory access in your
> case, it can still cause issues with IO access for some other
> platform.
> 
> Can you please test [1] and check it that works for you. You will need
> to define num-viewport in your device tree file.
> 
> ~Pratyush
> 
> [1] 
> https://github.com/pratyushanand/linux/commit/131b83ea7db0834d77ee5df65c6696bccbf8a1ce
> 
> .
> 

Checked, it works for us.

I think it would be better to exchange the assignments of MEMORYs and
CFGs/IOs when num_viewports <= 2, cause it fixes wrong memory access.

As you mentioned, other corner point for failure is still there while
there are only two viewports. It seems that there is not a perfect
solution.

Best Regards
Dong Bo

[PATCH] Exchange the Assignments of `MEMORYs' and `CFGs/IOs' in Designware PCIe Driver

2016-06-28 Thread dongbo (E) 
From: Dong Bo 

In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
When sending CFGs to peripherals (e.g. lspci), iatu0 frequently switches
between CFG and IO alternatively.

A MEMORY probably be sent as an IOs by mistake. Considering the following
configurations:
MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io

Suppose PCIe has just completed a CFG access, to switch back to IO, it set
the BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io. When another
CFG comes, the BASE_ADDR is set to 0xb400 to switch to CFG. At this
moment, a MEMORY access shows up, since it matches with iatu0
(due to 0xb400 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFF), it
is treated as an IO access by mistake, then sent to perpheral.

This patch fixes the problem by exchanging the assignments of `MEMORYs'
and `CFGs/IOs', which assigning MEMEORYs to iatu0, CFGs and IOs to iatu1.

Signed-off-by: Dong Bo 
---
 drivers/pci/host/pcie-designware.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index aafd766..1bd88d9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}
 -  dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);
 @@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}
 -  dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);
 @@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 * we should not program the ATU here.
 */
if (!pp->ops->rd_other_conf)
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
  PCIE_ATU_TYPE_MEM, pp->mem_base,
  pp->mem_bus_addr, pp->mem_size);
 -- 1.9.1


.

[PATCH] Exchange the Assignments of `MEMORYs' and `CFGs/IOs' in Designware PCIe Driver

2016-06-28 Thread dongbo (E) 
From: Dong Bo 

In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
When sending CFGs to peripherals (e.g. lspci), iatu0 frequently switches
between CFG and IO alternatively.

A MEMORY probably be sent as an IOs by mistake. Considering the following
configurations:
MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io

Suppose PCIe has just completed a CFG access, to switch back to IO, it set
the BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io. When another
CFG comes, the BASE_ADDR is set to 0xb400 to switch to CFG. At this
moment, a MEMORY access shows up, since it matches with iatu0
(due to 0xb400 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFF), it
is treated as an IO access by mistake, then sent to perpheral.

This patch fixes the problem by exchanging the assignments of `MEMORYs'
and `CFGs/IOs', which assigning MEMEORYs to iatu0, CFGs and IOs to iatu1.

Signed-off-by: Dong Bo 
---
 drivers/pci/host/pcie-designware.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index aafd766..1bd88d9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}
 -  dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);
 @@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}
 -  dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);
 @@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 * we should not program the ATU here.
 */
if (!pp->ops->rd_other_conf)
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
  PCIE_ATU_TYPE_MEM, pp->mem_base,
  pp->mem_bus_addr, pp->mem_size);
 -- 1.9.1


.

Re: [PATCH] Decouple CFG and IO in Designware PCIe Driver

2016-06-27 Thread dongbo (E) 
Hi, all.

How about exchanging the assignments of `MEMORYs' and `CFGs/IOs'?
In other words, assign MEMEORYs to iatu0, CFGs and IOs to iatu1.

Once the iatu0 is initialized to MEMORY accesses, its BASE_ADDR,
LIMIT and TYPE is fixed. MEMORYs match with iatu0 at first, so
they will never being treated as IOs or CFGs by mistake.

The number of viewpoints used remains two, it would be OK for
platforms only have 2 viewpoints.

Here is the patch:

Signed-off-by: Dong Bo <dong...@huawei.com>
---
 drivers/pci/host/pcie-designware.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index aafd766..1bd88d9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}

-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);

@@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}

-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);

@@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 * we should not program the ATU here.
 */
if (!pp->ops->rd_other_conf)
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
  PCIE_ATU_TYPE_MEM, pp->mem_base,
  pp->mem_bus_addr, pp->mem_size);

-- 
1.9.1

On 2016/6/27 12:37, Pratyush Anand wrote:
> On Wed, Jun 22, 2016 at 1:54 PM, dongbo (E) <dong...@huawei.com> wrote:
>> From: Dong Bo <dong...@huawei.com>
>>
>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
>> When PCIe sends CFGs to peripherals (e.g. lspci),
>> iatu0 frequently switches between CFG and IO alternatively.
>>
>> If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT,
>> this probably results in a MEMORY beging matched to be an IO by mistake.
>>
>> Considering the following configurations:
>> MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
>> CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
>> IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io
>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set 
>> the BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io.
>> When another CFG access come,
>> PCIe first set BASE_ADDR to 0xb400 to switch to CFG.
>> At this moment, a MEMORY access shows up, due to `0xb400 <= MEMORY 
>> BASE_ADDR <= MEMORY LIMIE <= 0xFFF, it matches with iatu0.
>> And it is treated as an IO access by mistake, then sent to perpheral.
>>
> 
> Hummm...This portion of driver has always been buggy.
> 
>> This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to 
>> IO.
> 
> But, we can not just assign IOs to iatu2.
> IIRC then, there are atleast two platforms which have only 2
> viewports, therefore they can not program iatu2.
> 
> Jingoo,Bjorn: IMHO, we should modify this portion of code, since more
> number of platforms has 4+ viewports. Probably, we can take following
> approach:
> 
> (1) Pass number of viewports through DT. If we have *atleast*  3
> viewports then assign separate viewports to memory and IO,  and share
> one with CFG0 and CFG1.
> (2) If we can have *atleast*  4 then, we may have separate for CFG0
> and CFG1 as well.
> 
> (3) If we have *only* 2 then,  either we let them work as they work
> today with

Re: [PATCH] Decouple CFG and IO in Designware PCIe Driver

2016-06-27 Thread dongbo (E) 
Hi, all.

How about exchanging the assignments of `MEMORYs' and `CFGs/IOs'?
In other words, assign MEMEORYs to iatu0, CFGs and IOs to iatu1.

Once the iatu0 is initialized to MEMORY accesses, its BASE_ADDR,
LIMIT and TYPE is fixed. MEMORYs match with iatu0 at first, so
they will never being treated as IOs or CFGs by mistake.

The number of viewpoints used remains two, it would be OK for
platforms only have 2 viewpoints.

Here is the patch:

Signed-off-by: Dong Bo 
---
 drivers/pci/host/pcie-designware.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index aafd766..1bd88d9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}

-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);

@@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
va_cfg_base = pp->va_cfg1_base;
}

-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_IO, pp->io_base,
  pp->io_bus_addr, pp->io_size);

@@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 * we should not program the ATU here.
 */
if (!pp->ops->rd_other_conf)
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
  PCIE_ATU_TYPE_MEM, pp->mem_base,
  pp->mem_bus_addr, pp->mem_size);

-- 
1.9.1

On 2016/6/27 12:37, Pratyush Anand wrote:
> On Wed, Jun 22, 2016 at 1:54 PM, dongbo (E)  wrote:
>> From: Dong Bo 
>>
>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
>> When PCIe sends CFGs to peripherals (e.g. lspci),
>> iatu0 frequently switches between CFG and IO alternatively.
>>
>> If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT,
>> this probably results in a MEMORY beging matched to be an IO by mistake.
>>
>> Considering the following configurations:
>> MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
>> CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
>> IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io
>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set 
>> the BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io.
>> When another CFG access come,
>> PCIe first set BASE_ADDR to 0xb400 to switch to CFG.
>> At this moment, a MEMORY access shows up, due to `0xb400 <= MEMORY 
>> BASE_ADDR <= MEMORY LIMIE <= 0xFFF, it matches with iatu0.
>> And it is treated as an IO access by mistake, then sent to perpheral.
>>
> 
> Hummm...This portion of driver has always been buggy.
> 
>> This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to 
>> IO.
> 
> But, we can not just assign IOs to iatu2.
> IIRC then, there are atleast two platforms which have only 2
> viewports, therefore they can not program iatu2.
> 
> Jingoo,Bjorn: IMHO, we should modify this portion of code, since more
> number of platforms has 4+ viewports. Probably, we can take following
> approach:
> 
> (1) Pass number of viewports through DT. If we have *atleast*  3
> viewports then assign separate viewports to memory and IO,  and share
> one with CFG0 and CFG1.
> (2) If we can have *atleast*  4 then, we may have separate for CFG0
> and CFG1 as well.
> 
> (3) If we have *only* 2 then,  either we let them work as they work
> today with bug, or may be we restrict them from using IO transactions.
> So assign one to

[PATCH] Decouple CFG and IO in Designware PCIe Driver

2016-06-22 Thread dongbo (E) 
From: Dong Bo 

In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
When PCIe sends CFGs to peripherals (e.g. lspci),
iatu0 frequently switches between CFG and IO alternatively.

If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT, 
this probably results in a MEMORY beging matched to be an IO by mistake.

Considering the following configurations:
MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io
Suppose PCIe has just completed a CFG access, to switch back to IO, it set the 
BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io.
When another CFG access come,
PCIe first set BASE_ADDR to 0xb400 to switch to CFG.
At this moment, a MEMORY access shows up, due to `0xb400 <= MEMORY 
BASE_ADDR <= MEMORY LIMIE <= 0xFFF, it matches with iatu0.
And it is treated as an IO access by mistake, then sent to perpheral.

This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to IO.

Signed-off-by: Dong Bo 
---
 drivers/pci/host/pcie-designware.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index aafd766..1a40305 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -51,6 +51,7 @@
 #define PCIE_ATU_VIEWPORT  0x900
 #define PCIE_ATU_REGION_INBOUND(0x1 << 31)
 #define PCIE_ATU_REGION_OUTBOUND   (0x0 << 31)
+#define PCIE_ATU_REGION_INDEX2 (0x2 << 0)
 #define PCIE_ATU_REGION_INDEX1 (0x1 << 0)
 #define PCIE_ATU_REGION_INDEX0 (0x0 << 0)
 #define PCIE_ATU_CR1   0x904
@@ -603,9 +604,6 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_IO, pp->io_base,
- pp->io_bus_addr, pp->io_size);
 
return ret;
 }
@@ -640,9 +638,6 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_IO, pp->io_base,
- pp->io_bus_addr, pp->io_size);
 
return ret;
 }
@@ -778,10 +773,15 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 * uses its own address translation component rather than ATU, so
 * we should not program the ATU here.
 */
-   if (!pp->ops->rd_other_conf)
+   if (!pp->ops->rd_other_conf) {
dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_MEM, pp->mem_base,
  pp->mem_bus_addr, pp->mem_size);
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX2,
+   PCIE_ATU_TYPE_IO, pp->io_base,
+   pp->io_bus_addr, pp->io_size);
+
+   }
 
dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
 
--
1.9.1

[PATCH] Decouple CFG and IO in Designware PCIe Driver

2016-06-22 Thread dongbo (E) 
From: Dong Bo 

In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
When PCIe sends CFGs to peripherals (e.g. lspci),
iatu0 frequently switches between CFG and IO alternatively.

If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT, 
this probably results in a MEMORY beging matched to be an IO by mistake.

Considering the following configurations:
MEMORY  ->  BASE_ADDR: 0xb410, LIMIT: 0xb4100FFF, TYPE=mem
CFG ->  BASE_ADDR: 0xb400, LIMIT: 0xb4000FFF, TYPE=cfg
IO  ->  BASE_ADDR: 0x, LIMIT: 0xFFFE, TYPE=io
Suppose PCIe has just completed a CFG access, to switch back to IO, it set the 
BASE_ADDR to 0x, LIMIT 0xFFFE and TYPE to io.
When another CFG access come,
PCIe first set BASE_ADDR to 0xb400 to switch to CFG.
At this moment, a MEMORY access shows up, due to `0xb400 <= MEMORY 
BASE_ADDR <= MEMORY LIMIE <= 0xFFF, it matches with iatu0.
And it is treated as an IO access by mistake, then sent to perpheral.

This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to IO.

Signed-off-by: Dong Bo 
---
 drivers/pci/host/pcie-designware.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index aafd766..1a40305 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -51,6 +51,7 @@
 #define PCIE_ATU_VIEWPORT  0x900
 #define PCIE_ATU_REGION_INBOUND(0x1 << 31)
 #define PCIE_ATU_REGION_OUTBOUND   (0x0 << 31)
+#define PCIE_ATU_REGION_INDEX2 (0x2 << 0)
 #define PCIE_ATU_REGION_INDEX1 (0x1 << 0)
 #define PCIE_ATU_REGION_INDEX0 (0x0 << 0)
 #define PCIE_ATU_CR1   0x904
@@ -603,9 +604,6 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_IO, pp->io_base,
- pp->io_bus_addr, pp->io_size);
 
return ret;
 }
@@ -640,9 +638,6 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, 
struct pci_bus *bus,
  type, cpu_addr,
  busdev, cfg_size);
ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
-   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_IO, pp->io_base,
- pp->io_bus_addr, pp->io_size);
 
return ret;
 }
@@ -778,10 +773,15 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 * uses its own address translation component rather than ATU, so
 * we should not program the ATU here.
 */
-   if (!pp->ops->rd_other_conf)
+   if (!pp->ops->rd_other_conf) {
dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
  PCIE_ATU_TYPE_MEM, pp->mem_base,
  pp->mem_bus_addr, pp->mem_size);
+   dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX2,
+   PCIE_ATU_TYPE_IO, pp->io_base,
+   pp->io_bus_addr, pp->io_size);
+
+   }
 
dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
 
--
1.9.1