Re: [PATCH v12.update2 02/15] PCI: Let pci_mmap_page_range() take resource address
On Fri, Jun 17, 2016 at 12:52 PM, Bjorn Helgaas wrote: >> >> and respin the whole patchset today. > > I added your acks and pushed the result to pci/resource. I'll also > post these formally on the list so they're easier to find. Please review patchset v13 that is against your new pci/resource branch. Thanks Yinghai
Re: [PATCH v12.update2 02/15] PCI: Let pci_mmap_page_range() take resource address
On Fri, Jun 17, 2016 at 12:25:49PM -0700, Yinghai Lu wrote:
> On Thu, Jun 16, 2016 at 7:15 PM, Bjorn Helgaas wrote:
> > On Thu, Jun 09, 2016 at 03:25:52PM -0700, Yinghai Lu wrote:
> >> In 8c05cd08a7 ("PCI: fix offset check for sysfs mmapped files"), try
> >> to check exposed value with resource start/end in proc mmap path.
> >>
> >> |start = vma->vm_pgoff;
> >> |size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
> >> |pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
> >> |pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
> >> |if (start >= pci_start && start < pci_start + size &&
> >> |start + nr <= pci_start + size)
> >>
> >> That breaks sparc that exposed value is BAR value, and need to be offseted
> >> to resource address.
> >
> > I'm not quite sure what you're saying here. Are you saying that sparc
> > is currently broken, and this patch fixes it? If so, what exactly is
> > broken? Can you give a small example of an mmap that is currently
> > broken?
> >
> >> Original pci_mmap_page_range() is taking PCI BAR value aka usr_address.
> >>
> >> Bjorn found out that it would be much simple to pass resource address
> >> directly and avoid extra those __pci_mmap_make_offset.
> >>
> >> In this patch:
> >> 1. in proc path: proc_bus_pci_mmap, try convert back to resource
> >>before calling pci_mmap_page_range
> >> 2. in sysfs path: pci_mmap_resource will just offset with resource start.
> >> 3. all pci_mmap_page_range will have vma->vm_pgoff with in resource
> >>range instead of BAR value.
> >> 4. remove __pci_mmap_make_offset, as the checking is done
> >>in pci_mmap_fits().
> >
> > This is a pretty big patch. It would help a lot to split it up.
>
> Looks like they are tight together after change api. vm_pgoff meaning changes.
>
> I could split item 4 to another patch, but compiler could complain or
> even refuse to
> go on if static functions are defined but not used.
Yeah, I was afraid they might be too tightly coupled to split up.
Still, every little bit helps.
> > I think the comment about "re-enabling the 2 lines below" is pointless
> > because doing that would break applications, which I don't think we'll
> > do.
> >
> > I propose the microblaze, powerpc, and sparc patches below, which
> > remove simplify pci_resource_to_user() and clean up this comment.
>
> Agreed. Actually I have the change for sparc/PCI in patch 3
>sparc/PCI: Use correct offset for bus address to resource
> according to previous review.
Sure enough, I see it there now. I think it's easier to review when
split out, so I'll keep it separate, since it's not actually dependent
on the rest of the changes in "sparc/PCI: Use correct offset for bus
address to resource".
> Will drop related change in sparc/PCI: Use correct offset for bus
> address to resource
>
> and respin the whole patchset today.
I added your acks and pushed the result to pci/resource. I'll also
post these formally on the list so they're easier to find.
Bjorn
Re: [PATCH v12.update2 02/15] PCI: Let pci_mmap_page_range() take resource address
On Thu, Jun 16, 2016 at 7:15 PM, Bjorn Helgaas wrote:
> On Thu, Jun 09, 2016 at 03:25:52PM -0700, Yinghai Lu wrote:
>> In 8c05cd08a7 ("PCI: fix offset check for sysfs mmapped files"), try
>> to check exposed value with resource start/end in proc mmap path.
>>
>> |start = vma->vm_pgoff;
>> |size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
>> |pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
>> |pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
>> |if (start >= pci_start && start < pci_start + size &&
>> |start + nr <= pci_start + size)
>>
>> That breaks sparc that exposed value is BAR value, and need to be offseted
>> to resource address.
>
> I'm not quite sure what you're saying here. Are you saying that sparc
> is currently broken, and this patch fixes it? If so, what exactly is
> broken? Can you give a small example of an mmap that is currently
> broken?
>
>> Original pci_mmap_page_range() is taking PCI BAR value aka usr_address.
>>
>> Bjorn found out that it would be much simple to pass resource address
>> directly and avoid extra those __pci_mmap_make_offset.
>>
>> In this patch:
>> 1. in proc path: proc_bus_pci_mmap, try convert back to resource
>>before calling pci_mmap_page_range
>> 2. in sysfs path: pci_mmap_resource will just offset with resource start.
>> 3. all pci_mmap_page_range will have vma->vm_pgoff with in resource
>>range instead of BAR value.
>> 4. remove __pci_mmap_make_offset, as the checking is done
>>in pci_mmap_fits().
>
> This is a pretty big patch. It would help a lot to split it up.
Looks like they are tight together after change api. vm_pgoff meaning changes.
I could split item 4 to another patch, but compiler could complain or
even refuse to
go on if static functions are defined but not used.
...
>
> I think the comment about "re-enabling the 2 lines below" is pointless
> because doing that would break applications, which I don't think we'll
> do.
>
> I propose the microblaze, powerpc, and sparc patches below, which
> remove simplify pci_resource_to_user() and clean up this comment.
Agreed. Actually I have the change for sparc/PCI in patch 3
sparc/PCI: Use correct offset for bus address to resource
according to previous review.
>> @@ -999,7 +1010,6 @@ static int pci_mmap_resource(struct kobject *kobj,
>> struct bin_attribute *attr,
>> struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
>> struct resource *res = attr->private;
>> enum pci_mmap_state mmap_type;
>> - resource_size_t start, end;
>> int i;
>>
>> for (i = 0; i < PCI_ROM_RESOURCE; i++)
>> @@ -1008,10 +1018,21 @@ static int pci_mmap_resource(struct kobject *kobj,
>> struct bin_attribute *attr,
>> if (i >= PCI_ROM_RESOURCE)
>> return -ENODEV;
>>
>> + /*
>> + * resource start have to be PAGE_SIZE aligned, as we pass
>> + * back virt address include round down of resource_start,
>> + * that caller can not figure out directly.
>> + * when it is not aligned, that mean it is io port, should go
>> + * pci_read_resource_io()/pci_write_resource_io() path.
>> + */
>> + if (res->start & ~PAGE_MASK)
>> + return -EINVAL;
>
> It seems reasonable to require that the mmap start and end be
> page-aligned. It seems like we ought to do the same for the sysfs and
> the procfs paths.
>
> Since we haven't enforced this in the past, there is the potential for
> breaking user programs, isn't there?
>
> The alignment enforcement should be in a patch by itself, so bisection
> would tell us something useful.
ok. will do that.
>
> commit 3dbd970b6d9a96ab471b4b86650a0200a47d375d
> Author: Bjorn Helgaas
> Date: Thu May 5 11:39:04 2016 -0500
>
> microblaze/PCI: Implement pci_resource_to_user() with
> pcibios_resource_to_bus()
>
> "User" addresses are shown in /sys/devices/pci.../.../resource and
> /proc/bus/pci/devices and used as mmap offsets for /proc/bus/pci/BB/DD.F
> files. For I/O port resources on microblaze, these are PCI bus addresses,
> i.e., raw BAR values.
>
> Previously pci_resource_to_user() computed the user address by subtracting
> "hose->io_base_virt - _IO_BASE" from the resource start:
>
> pci_resource_to_user()
> if (IO)
> offset = (unsigned long)hose->io_base_virt - _IO_BASE;
> *start = rsrc->start - offset;
>
> We've already told the PCI core about that "hose->io_base_virt - _IO_BASE"
> offset:
>
> pcibios_setup_phb_resources()
> res = &hose->io_resource;
> pci_add_resource_offset(resources, res, hose->io_base_virt -
> _IO_BASE);
>
> so pcibios_resource_to_bus() knows how to do that translation.
>
> No functional change intended.
>
> Signed-off-by: Bjorn Helgaas
Acked-by: Yinghai Lu
>
> diff --git a/arch/microblaze/pci/pci-common.c
> b/arch/microblaze/pci/pci-common.c
> index 1974567..e0dd64e 100
Re: [PATCH v12.update2 02/15] PCI: Let pci_mmap_page_range() take resource address
On Thu, Jun 09, 2016 at 03:25:52PM -0700, Yinghai Lu wrote:
> In 8c05cd08a7 ("PCI: fix offset check for sysfs mmapped files"), try
> to check exposed value with resource start/end in proc mmap path.
>
> |start = vma->vm_pgoff;
> |size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
> |pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
> |pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
> |if (start >= pci_start && start < pci_start + size &&
> |start + nr <= pci_start + size)
>
> That breaks sparc that exposed value is BAR value, and need to be offseted
> to resource address.
I'm not quite sure what you're saying here. Are you saying that sparc
is currently broken, and this patch fixes it? If so, what exactly is
broken? Can you give a small example of an mmap that is currently
broken?
> Original pci_mmap_page_range() is taking PCI BAR value aka usr_address.
>
> Bjorn found out that it would be much simple to pass resource address
> directly and avoid extra those __pci_mmap_make_offset.
>
> In this patch:
> 1. in proc path: proc_bus_pci_mmap, try convert back to resource
>before calling pci_mmap_page_range
> 2. in sysfs path: pci_mmap_resource will just offset with resource start.
> 3. all pci_mmap_page_range will have vma->vm_pgoff with in resource
>range instead of BAR value.
> 4. remove __pci_mmap_make_offset, as the checking is done
>in pci_mmap_fits().
This is a pretty big patch. It would help a lot to split it up.
> -v2: add pci_user_to_resource() and remove __pci_mmap_make_offset()
> -v4: update after three patches with __pci_mmap_set_pgprot()
>
> Signed-off-by: Yinghai Lu
> Cc: linuxppc-...@lists.ozlabs.org
> Cc: sparcli...@vger.kernel.org
> Cc: linux-xte...@linux-xtensa.org
> ---
> arch/microblaze/pci/pci-common.c | 78 +++---
> arch/powerpc/kernel/pci-common.c | 78 +++---
> arch/sparc/kernel/pci.c | 117
> ---
> arch/xtensa/kernel/pci.c | 75 -
> drivers/pci/pci-sysfs.c | 33 ---
> drivers/pci/pci.h| 2 +-
> drivers/pci/proc.c | 60 +---
> 7 files changed, 103 insertions(+), 340 deletions(-)
>
> diff --git a/arch/microblaze/pci/pci-common.c
> b/arch/microblaze/pci/pci-common.c
> index 1974567..881249f 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -156,69 +156,6 @@ void pcibios_set_master(struct pci_dev *dev)
> */
>
> /*
> - * Adjust vm_pgoff of VMA such that it is the physical page offset
> - * corresponding to the 32-bit pci bus offset for DEV requested by the user.
> - *
> - * Basically, the user finds the base address for his device which he wishes
> - * to mmap. They read the 32-bit value from the config space base register,
> - * add whatever PAGE_SIZE multiple offset they wish, and feed this into the
> - * offset parameter of mmap on /proc/bus/pci/XXX for that device.
> - *
> - * Returns negative error code on failure, zero on success.
> - */
> -static struct resource *__pci_mmap_make_offset(struct pci_dev *dev,
> -resource_size_t *offset,
> -enum pci_mmap_state mmap_state)
> -{
> - struct pci_controller *hose = pci_bus_to_host(dev->bus);
> - unsigned long io_offset = 0;
> - int i, res_bit;
> -
> - if (!hose)
> - return NULL;/* should never happen */
> -
> - /* If memory, add on the PCI bridge address offset */
> - if (mmap_state == pci_mmap_mem) {
> -#if 0 /* See comment in pci_resource_to_user() for why this is disabled */
> - *offset += hose->pci_mem_offset;
> -#endif
> - res_bit = IORESOURCE_MEM;
> - } else {
> - io_offset = (unsigned long)hose->io_base_virt - _IO_BASE;
> - *offset += io_offset;
> - res_bit = IORESOURCE_IO;
> - }
> -
> - /*
> - * Check that the offset requested corresponds to one of the
> - * resources of the device.
> - */
> - for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
> - struct resource *rp = &dev->resource[i];
> - int flags = rp->flags;
> -
> - /* treat ROM as memory (should be already) */
> - if (i == PCI_ROM_RESOURCE)
> - flags |= IORESOURCE_MEM;
> -
> - /* Active and same type? */
> - if ((flags & res_bit) == 0)
> - continue;
> -
> - /* In the range of this resource? */
> - if (*offset < (rp->start & PAGE_MASK) || *offset > rp->end)
> - continue;
If you can, please split the validation removal into a separate patch,
so we can easily review and make sure the equivalent validation is
being done in pci_mmap_fits() (or wherever it is).
> -
