From: Eric Auger <eric.auger@linaro.org>
To: Baptiste Reynal <b.reynal@virtualopensystems.com>,
James Morse <james.morse@arm.com>
Cc: "open list:VFIO DRIVER" <kvm@vger.kernel.org>,
Alex Williamson <alex.williamson@redhat.com>,
Antonios Motakis <a.motakis@virtualopensystems.com>
Subject: Re: [PATCH] vfio/platform: store mapped memory in region, instead of an on-stack copy
Date: Fri, 30 Oct 2015 10:19:39 +0100 [thread overview]
Message-ID: <5633362B.70109@linaro.org> (raw)
In-Reply-To: <CAN9JPjEZJ49o9mA4Grkgpsojsi5EtXPniQZMYU3KD1ADtRMduQ@mail.gmail.com>
Hi,
On 10/30/2015 09:51 AM, Baptiste Reynal wrote:
> Hi James,
>
> Thanks for this fix.
>
> Acked-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> Tested-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
>
> On Thu, Oct 29, 2015 at 5:50 PM, James Morse <james.morse@arm.com> wrote:
>> vfio_platform_{read,write}_mmio() call ioremap_nocache() to map
>> a region of io memory, which they store in struct vfio_platform_region to
>> be eventually re-used, or unmapped by vfio_platform_regions_cleanup().
>>
>> These functions receive a copy of their struct vfio_platform_region
>> argument on the stack - so these mapped areas are always allocated, and
>> always leaked.
I just noticed I have a leak in reset modules too. I am going to correct
this.
Thanks
Eric
>>
>> Pass this argument as a pointer instead.
>>
>> Fixes: 6e3f26456009 "vfio/platform: read and write support for the device fd"
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 36 ++++++++++++++--------------
>> 1 file changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index f3b6299..ccf5da5 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -308,17 +308,17 @@ static long vfio_platform_ioctl(void *device_data,
>> return -ENOTTY;
>> }
>>
>> -static ssize_t vfio_platform_read_mmio(struct vfio_platform_region reg,
>> +static ssize_t vfio_platform_read_mmio(struct vfio_platform_region *reg,
>> char __user *buf, size_t count,
>> loff_t off)
>> {
>> unsigned int done = 0;
>>
>> - if (!reg.ioaddr) {
>> - reg.ioaddr =
>> - ioremap_nocache(reg.addr, reg.size);
>> + if (!reg->ioaddr) {
>> + reg->ioaddr =
>> + ioremap_nocache(reg->addr, reg->size);
>>
>> - if (!reg.ioaddr)
>> + if (!reg->ioaddr)
>> return -ENOMEM;
>> }
>>
>> @@ -328,7 +328,7 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region reg,
>> if (count >= 4 && !(off % 4)) {
>> u32 val;
>>
>> - val = ioread32(reg.ioaddr + off);
>> + val = ioread32(reg->ioaddr + off);
>> if (copy_to_user(buf, &val, 4))
>> goto err;
>>
>> @@ -336,7 +336,7 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region reg,
>> } else if (count >= 2 && !(off % 2)) {
>> u16 val;
>>
>> - val = ioread16(reg.ioaddr + off);
>> + val = ioread16(reg->ioaddr + off);
>> if (copy_to_user(buf, &val, 2))
>> goto err;
>>
>> @@ -344,7 +344,7 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region reg,
>> } else {
>> u8 val;
>>
>> - val = ioread8(reg.ioaddr + off);
>> + val = ioread8(reg->ioaddr + off);
>> if (copy_to_user(buf, &val, 1))
>> goto err;
>>
>> @@ -377,7 +377,7 @@ static ssize_t vfio_platform_read(void *device_data, char __user *buf,
>> return -EINVAL;
>>
>> if (vdev->regions[index].type & VFIO_PLATFORM_REGION_TYPE_MMIO)
>> - return vfio_platform_read_mmio(vdev->regions[index],
>> + return vfio_platform_read_mmio(&vdev->regions[index],
>> buf, count, off);
>> else if (vdev->regions[index].type & VFIO_PLATFORM_REGION_TYPE_PIO)
>> return -EINVAL; /* not implemented */
>> @@ -385,17 +385,17 @@ static ssize_t vfio_platform_read(void *device_data, char __user *buf,
>> return -EINVAL;
>> }
>>
>> -static ssize_t vfio_platform_write_mmio(struct vfio_platform_region reg,
>> +static ssize_t vfio_platform_write_mmio(struct vfio_platform_region *reg,
>> const char __user *buf, size_t count,
>> loff_t off)
>> {
>> unsigned int done = 0;
>>
>> - if (!reg.ioaddr) {
>> - reg.ioaddr =
>> - ioremap_nocache(reg.addr, reg.size);
>> + if (!reg->ioaddr) {
>> + reg->ioaddr =
>> + ioremap_nocache(reg->addr, reg->size);
>>
>> - if (!reg.ioaddr)
>> + if (!reg->ioaddr)
>> return -ENOMEM;
>> }
>>
>> @@ -407,7 +407,7 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region reg,
>>
>> if (copy_from_user(&val, buf, 4))
>> goto err;
>> - iowrite32(val, reg.ioaddr + off);
>> + iowrite32(val, reg->ioaddr + off);
>>
>> filled = 4;
>> } else if (count >= 2 && !(off % 2)) {
>> @@ -415,7 +415,7 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region reg,
>>
>> if (copy_from_user(&val, buf, 2))
>> goto err;
>> - iowrite16(val, reg.ioaddr + off);
>> + iowrite16(val, reg->ioaddr + off);
>>
>> filled = 2;
>> } else {
>> @@ -423,7 +423,7 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region reg,
>>
>> if (copy_from_user(&val, buf, 1))
>> goto err;
>> - iowrite8(val, reg.ioaddr + off);
>> + iowrite8(val, reg->ioaddr + off);
>>
>> filled = 1;
>> }
>> @@ -453,7 +453,7 @@ static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
>> return -EINVAL;
>>
>> if (vdev->regions[index].type & VFIO_PLATFORM_REGION_TYPE_MMIO)
>> - return vfio_platform_write_mmio(vdev->regions[index],
>> + return vfio_platform_write_mmio(&vdev->regions[index],
>> buf, count, off);
>> else if (vdev->regions[index].type & VFIO_PLATFORM_REGION_TYPE_PIO)
>> return -EINVAL; /* not implemented */
>> --
>> 2.6.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
prev parent reply other threads:[~2015-10-30 9:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-29 16:50 [PATCH] vfio/platform: store mapped memory in region, instead of an on-stack copy James Morse
2015-10-30 8:51 ` Baptiste Reynal
2015-10-30 9:19 ` Eric Auger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5633362B.70109@linaro.org \
--to=eric.auger@linaro.org \
--cc=a.motakis@virtualopensystems.com \
--cc=alex.williamson@redhat.com \
--cc=b.reynal@virtualopensystems.com \
--cc=james.morse@arm.com \
--cc=kvm@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.