Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Wed, Apr 29, 2020 at 06:20:48AM -0400, Michael S. Tsirkin wrote: > On Wed, Apr 29, 2020 at 03:39:53PM +0530, Srivatsa Vaddagiri wrote: > > That would still not work I think where swiotlb is used for pass-thr devices > > (when private memory is fine) as well as virtio devices (when shared memory > > is > > required). > > So that is a separate question. When there are multiple untrusted > devices, at the moment it looks like a single bounce buffer is used. > > Which to me seems like a security problem, I think we should protect > untrusted devices from each other. There are two DMA pools code in Linux already - the TTM one for graphics and the mm/dmapool.c - could those be used instead? Or augmented at least?
Re: [PATCH 5/5] virtio: Add bounce DMA ops
Hi Srivatsa, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on vhost/linux-next] [also build test WARNING on xen-tip/linux-next linus/master v5.7-rc3 next-20200428] [cannot apply to swiotlb/linux-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Srivatsa-Vaddagiri/virtio-on-Type-1-hypervisor/20200429-032334 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next reproduce: # apt-get install sparse # sparse version: v0.6.1-191-gc51a0382-dirty make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot sparse warnings: (new ones prefixed by >>) >> drivers/virtio/virtio_bounce.c:22:21: sparse: sparse: symbol 'virtio_pool' >> was not declared. Should it be static? >> drivers/virtio/virtio_bounce.c:79:8: sparse: sparse: symbol >> 'virtio_max_mapping_size' was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [virtio-dev] Re: [PATCH 5/5] virtio: Add bounce DMA ops
On 29.04.20 12:45, Michael S. Tsirkin wrote: On Wed, Apr 29, 2020 at 12:26:43PM +0200, Jan Kiszka wrote: On 29.04.20 12:20, Michael S. Tsirkin wrote: On Wed, Apr 29, 2020 at 03:39:53PM +0530, Srivatsa Vaddagiri wrote: That would still not work I think where swiotlb is used for pass-thr devices (when private memory is fine) as well as virtio devices (when shared memory is required). So that is a separate question. When there are multiple untrusted devices, at the moment it looks like a single bounce buffer is used. Which to me seems like a security problem, I think we should protect untrusted devices from each other. Definitely. That's the model we have for ivshmem-virtio as well. Jan Want to try implementing that? The desire is definitely there, currently "just" not the time. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Wed, Apr 29, 2020 at 12:26:43PM +0200, Jan Kiszka wrote: > On 29.04.20 12:20, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 03:39:53PM +0530, Srivatsa Vaddagiri wrote: > > > That would still not work I think where swiotlb is used for pass-thr > > > devices > > > (when private memory is fine) as well as virtio devices (when shared > > > memory is > > > required). > > > > So that is a separate question. When there are multiple untrusted > > devices, at the moment it looks like a single bounce buffer is used. > > > > Which to me seems like a security problem, I think we should protect > > untrusted devices from each other. > > > > Definitely. That's the model we have for ivshmem-virtio as well. > > Jan Want to try implementing that? > -- > Siemens AG, Corporate Technology, CT RDA IOT SES-DE > Corporate Competence Center Embedded Linux
Re: [PATCH 5/5] virtio: Add bounce DMA ops
* Michael S. Tsirkin [2020-04-29 06:20:48]: > On Wed, Apr 29, 2020 at 03:39:53PM +0530, Srivatsa Vaddagiri wrote: > > That would still not work I think where swiotlb is used for pass-thr devices > > (when private memory is fine) as well as virtio devices (when shared memory > > is > > required). > > So that is a separate question. When there are multiple untrusted > devices, at the moment it looks like a single bounce buffer is used. > > Which to me seems like a security problem, I think we should protect > untrusted devices from each other. I think as first step, let me see if we can make swiotlb driver accept a target memory segment as its working area. That may suffice our needs I think. A subsequent step could be to make swiotlb driver recognize multiple pools. -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On 29.04.20 12:20, Michael S. Tsirkin wrote: On Wed, Apr 29, 2020 at 03:39:53PM +0530, Srivatsa Vaddagiri wrote: That would still not work I think where swiotlb is used for pass-thr devices (when private memory is fine) as well as virtio devices (when shared memory is required). So that is a separate question. When there are multiple untrusted devices, at the moment it looks like a single bounce buffer is used. Which to me seems like a security problem, I think we should protect untrusted devices from each other. Definitely. That's the model we have for ivshmem-virtio as well. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Wed, Apr 29, 2020 at 03:39:53PM +0530, Srivatsa Vaddagiri wrote: > That would still not work I think where swiotlb is used for pass-thr devices > (when private memory is fine) as well as virtio devices (when shared memory is > required). So that is a separate question. When there are multiple untrusted devices, at the moment it looks like a single bounce buffer is used. Which to me seems like a security problem, I think we should protect untrusted devices from each other. > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
* Michael S. Tsirkin [2020-04-29 05:52:05]: > > > So it seems that with modern Linux, all one needs > > > to do on x86 is mark the device as untrusted. > > > It's already possible to do this with ACPI and with OF - would that be > > > sufficient for achieving what this patchset is trying to do? > > > > In my case, its not sufficient to just mark virtio device untrusted and thus > > activate the use of swiotlb. All of the secondary VM memory, including those > > allocate by swiotlb driver, is private to it. > > So why not make the bounce buffer memory shared then? Its a limitation by our hypervisor. When a secondary VM is created, two memory segments are allocated - one private and other shared. There is no provision for the secondary VM to make part of its private memory shared after it boots. I can perhaps consider a change in swiotlb driver to accept the second shared memory segment as its main working area (rather than allocate its own). That would still not work I think where swiotlb is used for pass-thr devices (when private memory is fine) as well as virtio devices (when shared memory is required). -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Wed, Apr 29, 2020 at 03:14:10PM +0530, Srivatsa Vaddagiri wrote: > * Michael S. Tsirkin [2020-04-29 02:50:41]: > > > So it seems that with modern Linux, all one needs > > to do on x86 is mark the device as untrusted. > > It's already possible to do this with ACPI and with OF - would that be > > sufficient for achieving what this patchset is trying to do? > > In my case, its not sufficient to just mark virtio device untrusted and thus > activate the use of swiotlb. All of the secondary VM memory, including those > allocate by swiotlb driver, is private to it. So why not make the bounce buffer memory shared then? > An additional piece of memory is > available to secondary VM which is shared between VMs and which is where I > need > swiotlb driver to do its work. > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
* Michael S. Tsirkin [2020-04-29 02:50:41]: > So it seems that with modern Linux, all one needs > to do on x86 is mark the device as untrusted. > It's already possible to do this with ACPI and with OF - would that be > sufficient for achieving what this patchset is trying to do? In my case, its not sufficient to just mark virtio device untrusted and thus activate the use of swiotlb. All of the secondary VM memory, including those allocate by swiotlb driver, is private to it. An additional piece of memory is available to secondary VM which is shared between VMs and which is where I need swiotlb driver to do its work. -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On 2020/4/29 14:50, Michael S. Tsirkin wrote: On Wed, Apr 29, 2020 at 01:42:13PM +0800, Lu Baolu wrote: On 2020/4/29 12:57, Michael S. Tsirkin wrote: On Wed, Apr 29, 2020 at 10:22:32AM +0800, Lu Baolu wrote: On 2020/4/29 4:41, Michael S. Tsirkin wrote: On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: * Michael S. Tsirkin [2020-04-28 12:17:57]: Okay, but how is all this virtio specific? For example, why not allow separate swiotlbs for any type of device? For example, this might make sense if a given device is from a different, less trusted vendor. Is swiotlb commonly used for multiple devices that may be on different trust boundaries (and not behind a hardware iommu)? Even a hardware iommu does not imply a 100% security from malicious hardware. First lots of people use iommu=pt for performance reasons. Second even without pt, unmaps are often batched, and sub-page buffers might be used for DMA, so we are not 100% protected at all times. For untrusted devices, IOMMU is forced on even iommu=pt is used; I think you are talking about untrusted*drivers* like with VFIO. No. I am talking about untrusted devices like thunderbolt peripherals. We always trust drivers hosted in kernel and the DMA APIs are designed for them, right? Please refer to this series. https://lkml.org/lkml/2019/9/6/39 Best regards, baolu Oh, thanks for that! I didn't realize Linux is doing this. So it seems that with modern Linux, all one needs to do on x86 is mark the device as untrusted. It's already possible to do this with ACPI and with OF - would that be sufficient for achieving what this patchset is trying to do? Yes. Adding more ways to mark a device as untrusted, and adding support for more platforms to use bounce buffers sounds like a reasonable thing to do. Agreed. Best regards, baolu
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Wed, Apr 29, 2020 at 01:42:13PM +0800, Lu Baolu wrote: > On 2020/4/29 12:57, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 10:22:32AM +0800, Lu Baolu wrote: > > > On 2020/4/29 4:41, Michael S. Tsirkin wrote: > > > > On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: > > > > > * Michael S. Tsirkin [2020-04-28 12:17:57]: > > > > > > > > > > > Okay, but how is all this virtio specific? For example, why not > > > > > > allow > > > > > > separate swiotlbs for any type of device? > > > > > > For example, this might make sense if a given device is from a > > > > > > different, less trusted vendor. > > > > > Is swiotlb commonly used for multiple devices that may be on > > > > > different trust > > > > > boundaries (and not behind a hardware iommu)? > > > > Even a hardware iommu does not imply a 100% security from malicious > > > > hardware. First lots of people use iommu=pt for performance reasons. > > > > Second even without pt, unmaps are often batched, and sub-page buffers > > > > might be used for DMA, so we are not 100% protected at all times. > > > > > > > > > > For untrusted devices, IOMMU is forced on even iommu=pt is used; > > > > I think you are talking about untrusted *drivers* like with VFIO. > > No. I am talking about untrusted devices like thunderbolt peripherals. > We always trust drivers hosted in kernel and the DMA APIs are designed > for them, right? > > Please refer to this series. > > https://lkml.org/lkml/2019/9/6/39 > > Best regards, > baolu Oh, thanks for that! I didn't realize Linux is doing this. So it seems that with modern Linux, all one needs to do on x86 is mark the device as untrusted. It's already possible to do this with ACPI and with OF - would that be sufficient for achieving what this patchset is trying to do? Adding more ways to mark a device as untrusted, and adding support for more platforms to use bounce buffers sounds like a reasonable thing to do. > > > > On the other hand, I am talking about things like thunderbolt > > peripherals being less trusted than on-board ones. > > > > > > > Or possibly even using swiotlb for specific use-cases where > > speed is less of an issue. > > > > E.g. my wifi is pretty slow anyway, and that card is exposed to > > malicious actors all the time, put just that behind swiotlb > > for security, and leave my graphics card with pt since > > I'm trusting it with secrets anyway. > > > > > > > and > > > iotlb flush is in strict mode (no batched flushes); ATS is also not > > > allowed. Swiotlb is used to protect sub-page buffers since IOMMU can > > > only apply page granularity protection. Swiotlb is now used for devices > > > from different trust zone. > > > > > > Best regards, > > > baolu > >
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On 2020/4/29 12:57, Michael S. Tsirkin wrote: On Wed, Apr 29, 2020 at 10:22:32AM +0800, Lu Baolu wrote: On 2020/4/29 4:41, Michael S. Tsirkin wrote: On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: * Michael S. Tsirkin [2020-04-28 12:17:57]: Okay, but how is all this virtio specific? For example, why not allow separate swiotlbs for any type of device? For example, this might make sense if a given device is from a different, less trusted vendor. Is swiotlb commonly used for multiple devices that may be on different trust boundaries (and not behind a hardware iommu)? Even a hardware iommu does not imply a 100% security from malicious hardware. First lots of people use iommu=pt for performance reasons. Second even without pt, unmaps are often batched, and sub-page buffers might be used for DMA, so we are not 100% protected at all times. For untrusted devices, IOMMU is forced on even iommu=pt is used; I think you are talking about untrusted *drivers* like with VFIO. No. I am talking about untrusted devices like thunderbolt peripherals. We always trust drivers hosted in kernel and the DMA APIs are designed for them, right? Please refer to this series. https://lkml.org/lkml/2019/9/6/39 Best regards, baolu On the other hand, I am talking about things like thunderbolt peripherals being less trusted than on-board ones. Or possibly even using swiotlb for specific use-cases where speed is less of an issue. E.g. my wifi is pretty slow anyway, and that card is exposed to malicious actors all the time, put just that behind swiotlb for security, and leave my graphics card with pt since I'm trusting it with secrets anyway. and iotlb flush is in strict mode (no batched flushes); ATS is also not allowed. Swiotlb is used to protect sub-page buffers since IOMMU can only apply page granularity protection. Swiotlb is now used for devices from different trust zone. Best regards, baolu
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Wed, Apr 29, 2020 at 10:22:32AM +0800, Lu Baolu wrote: > On 2020/4/29 4:41, Michael S. Tsirkin wrote: > > On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: > > > * Michael S. Tsirkin [2020-04-28 12:17:57]: > > > > > > > Okay, but how is all this virtio specific? For example, why not allow > > > > separate swiotlbs for any type of device? > > > > For example, this might make sense if a given device is from a > > > > different, less trusted vendor. > > > Is swiotlb commonly used for multiple devices that may be on different > > > trust > > > boundaries (and not behind a hardware iommu)? > > Even a hardware iommu does not imply a 100% security from malicious > > hardware. First lots of people use iommu=pt for performance reasons. > > Second even without pt, unmaps are often batched, and sub-page buffers > > might be used for DMA, so we are not 100% protected at all times. > > > > For untrusted devices, IOMMU is forced on even iommu=pt is used; I think you are talking about untrusted *drivers* like with VFIO. On the other hand, I am talking about things like thunderbolt peripherals being less trusted than on-board ones. Or possibly even using swiotlb for specific use-cases where speed is less of an issue. E.g. my wifi is pretty slow anyway, and that card is exposed to malicious actors all the time, put just that behind swiotlb for security, and leave my graphics card with pt since I'm trusting it with secrets anyway. > and > iotlb flush is in strict mode (no batched flushes); ATS is also not > allowed. Swiotlb is used to protect sub-page buffers since IOMMU can > only apply page granularity protection. Swiotlb is now used for devices > from different trust zone. > > Best regards, > baolu
Re: [PATCH 5/5] virtio: Add bounce DMA ops
* Stefano Stabellini [2020-04-28 16:04:34]:
> > > Is swiotlb commonly used for multiple devices that may be on different
> > > trust
> > > boundaries (and not behind a hardware iommu)?
>
> The trust boundary is not a good way of describing the scenario and I
> think it leads to miscommunication.
>
> A better way to describe the scenario would be that the device can only
> DMA to/from a small reserved-memory region advertised on device tree.
>
> Do we have other instances of devices that can only DMA to/from very
> specific and non-configurable address ranges? If so, this series could
> follow their example.
AFAICT there is no such notion in current DMA API.
static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size,
bool is_ram)
{
return end <= min_not_zero(*dev->dma_mask, dev->bus_dma_limit);
}
Only the max address a device can access is defined and not a range that we seem
to need here. I think we need to set the bus_dma_limit to 0 for virtio devices
which will force the use of swiotlb_map API. We should also have a per-device
swiotlb pool defined, so that swiotlb can use the pool meant for the given
device.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
* Michael S. Tsirkin [2020-04-28 16:41:04]:
> > Won't we still need some changes to virtio to make use of its own pool (to
> > bounce buffers)? Something similar to its own DMA ops proposed in this
> > patch?
>
> If you are doing this for all devices, you need to either find a way
> to do this without chaning DMA ops, or by doing some automatic change
> to all drivers.
Ok thanks for this input. I will see how we can obfuscate this in DMA APIs
itself.
Can you also comment on the virtio transport problem I cited? The hypervisor we
are dealing with does not support MMIO transport. It supports message queue
send/recv and also doorbell, which I think can be used if we can make some
change like this to virtio_mmio.c:
+static inline u32
+virtio_readl(struct virtio_mmio_device *vm_dev, u32 reg_offset)
+{
+return vm_dev->mmio_ops->readl(vm_dev, reg_offset);
+}
+
+static inline void
+virtio_writel(struct virtio_mmio_device *vm_dev, u32 reg_offset, u32 data)
+{
+vm_dev->mmio_ops->writel(vm_dev, reg_offset, data);
+}
/* Check magic value */
-magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
+magic = vrito_readl(vm_dev, VIRTIO_MMIO_MAGIC_VALUE);
mmio_ops->readl on most platforms can default to readl itself, while on a
platform like us, it can boil down to message_queue send/recv. Would such a
change be acceptable?
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On 2020/4/29 4:41, Michael S. Tsirkin wrote: On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: * Michael S. Tsirkin [2020-04-28 12:17:57]: Okay, but how is all this virtio specific? For example, why not allow separate swiotlbs for any type of device? For example, this might make sense if a given device is from a different, less trusted vendor. Is swiotlb commonly used for multiple devices that may be on different trust boundaries (and not behind a hardware iommu)? Even a hardware iommu does not imply a 100% security from malicious hardware. First lots of people use iommu=pt for performance reasons. Second even without pt, unmaps are often batched, and sub-page buffers might be used for DMA, so we are not 100% protected at all times. For untrusted devices, IOMMU is forced on even iommu=pt is used; and iotlb flush is in strict mode (no batched flushes); ATS is also not allowed. Swiotlb is used to protect sub-page buffers since IOMMU can only apply page granularity protection. Swiotlb is now used for devices from different trust zone. Best regards, baolu
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, 28 Apr 2020, Michael S. Tsirkin wrote: > On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: > > * Michael S. Tsirkin [2020-04-28 12:17:57]: > > > > > Okay, but how is all this virtio specific? For example, why not allow > > > separate swiotlbs for any type of device? > > > For example, this might make sense if a given device is from a > > > different, less trusted vendor. > > > > Is swiotlb commonly used for multiple devices that may be on different trust > > boundaries (and not behind a hardware iommu)? The trust boundary is not a good way of describing the scenario and I think it leads to miscommunication. A better way to describe the scenario would be that the device can only DMA to/from a small reserved-memory region advertised on device tree. Do we have other instances of devices that can only DMA to/from very specific and non-configurable address ranges? If so, this series could follow their example. > Even a hardware iommu does not imply a 100% security from malicious > hardware. First lots of people use iommu=pt for performance reasons. > Second even without pt, unmaps are often batched, and sub-page buffers > might be used for DMA, so we are not 100% protected at all times.
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, 28 Apr 2020, Srivatsa Vaddagiri wrote:
> For better security, its desirable that a guest VM's memory is
> not accessible to any entity that executes outside the context of
> guest VM. In case of virtio, backend drivers execute outside the
> context of guest VM and in general will need access to complete
> guest VM memory. One option to restrict the access provided to
> backend driver is to make use of a bounce buffer. The bounce
> buffer is accessible to both backend and frontend drivers. All IO
> buffers that are in private space of guest VM are bounced to be
> accessible to backend.
[...]
> +static int __init virtio_bounce_setup(struct reserved_mem *rmem)
> +{
> + unsigned long node = rmem->fdt_node;
> +
> + if (!of_get_flat_dt_prop(node, "no-map", NULL))
> + return -EINVAL;
> +
> + return virtio_register_bounce_buffer(rmem->base, rmem->size);
> +}
> +
> +RESERVEDMEM_OF_DECLARE(virtio, "virtio_bounce_pool", virtio_bounce_setup);
Is this special reserved-memory region documented somewhere?
Re: [PATCH 5/5] virtio: Add bounce DMA ops
Hi Srivatsa,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on vhost/linux-next]
[also build test ERROR on xen-tip/linux-next linus/master v5.7-rc3
next-20200428]
[cannot apply to swiotlb/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Srivatsa-Vaddagiri/virtio-on-Type-1-hypervisor/20200429-032334
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: s390-randconfig-a001-20200428 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/virtio/virtio_bounce.c: In function 'virtio_bounce_setup':
>> drivers/virtio/virtio_bounce.c:144:7: error: implicit declaration of
>> function 'of_get_flat_dt_prop' [-Werror=implicit-function-declaration]
144 | if (!of_get_flat_dt_prop(node, "no-map", NULL))
| ^~~
cc1: some warnings being treated as errors
vim +/of_get_flat_dt_prop +144 drivers/virtio/virtio_bounce.c
139
140 static int __init virtio_bounce_setup(struct reserved_mem *rmem)
141 {
142 unsigned long node = rmem->fdt_node;
143
> 144 if (!of_get_flat_dt_prop(node, "no-map", NULL))
145 return -EINVAL;
146
147 return virtio_register_bounce_buffer(rmem->base, rmem->size);
148 }
149
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH 5/5] virtio: Add bounce DMA ops
Hi Srivatsa,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on vhost/linux-next]
[also build test WARNING on xen-tip/linux-next linus/master v5.7-rc3
next-20200428]
[cannot apply to swiotlb/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Srivatsa-Vaddagiri/virtio-on-Type-1-hypervisor/20200429-032334
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: sh-randconfig-a001-20200428 (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
In file included from drivers/virtio/virtio_bounce.c:13:
include/linux/swiotlb.h: In function 'swiotlb_alloc':
include/linux/swiotlb.h:234:9: error: 'DMA_MAPPING_ERROR' undeclared (first
use in this function)
234 | return DMA_MAPPING_ERROR;
| ^
include/linux/swiotlb.h:234:9: note: each undeclared identifier is reported
only once for each function it appears in
>> include/linux/swiotlb.h:235:1: warning: control reaches end of non-void
>> function [-Wreturn-type]
235 | }
| ^
vim +235 include/linux/swiotlb.h
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 229
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 230 static inline phys_addr_t
swiotlb_alloc(struct swiotlb_pool *pool,
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 231 size_t
alloc_size, unsigned long tbl_dma_addr,
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 232 unsigned long
mask)
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 233 {
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 234 return
DMA_MAPPING_ERROR;
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 @235 }
9ab4c39d9f9298 Srivatsa Vaddagiri 2020-04-28 236
:: The code at line 235 was first introduced by commit
:: 9ab4c39d9f929840cb884e589f6112770dbc2f63 swiotlb: Add alloc and free APIs
:: TO: Srivatsa Vaddagiri
:: CC: 0day robot
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote:
> * Michael S. Tsirkin [2020-04-28 12:17:57]:
>
> > Okay, but how is all this virtio specific? For example, why not allow
> > separate swiotlbs for any type of device?
> > For example, this might make sense if a given device is from a
> > different, less trusted vendor.
>
> Is swiotlb commonly used for multiple devices that may be on different trust
> boundaries (and not behind a hardware iommu)?
Even a hardware iommu does not imply a 100% security from malicious
hardware. First lots of people use iommu=pt for performance reasons.
Second even without pt, unmaps are often batched, and sub-page buffers
might be used for DMA, so we are not 100% protected at all times.
> If so, then yes it sounds like a
> good application of multiple swiotlb pools.
>
> > All this can then maybe be hidden behind the DMA API.
>
> Won't we still need some changes to virtio to make use of its own pool (to
> bounce buffers)? Something similar to its own DMA ops proposed in this patch?
If you are doing this for all devices, you need to either find a way
to do this without chaning DMA ops, or by doing some automatic change
to all drivers.
> > > +void virtio_bounce_set_dma_ops(struct virtio_device *vdev)
> > > +{
> > > + if (!bounce_buf_paddr)
> > > + return;
> > > +
> > > + set_dma_ops(vdev->dev.parent, &virtio_dma_ops);
> >
> >
> > I don't think DMA API maintainers will be happy with new users
> > of set_dma_ops.
>
> Is there an alternate API that is more preffered?
all this is supposed to be part of DMA API itself. new drivers aren't
supposed to have custom DMA ops.
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
* Michael S. Tsirkin [2020-04-28 12:17:57]:
> Okay, but how is all this virtio specific? For example, why not allow
> separate swiotlbs for any type of device?
> For example, this might make sense if a given device is from a
> different, less trusted vendor.
Is swiotlb commonly used for multiple devices that may be on different trust
boundaries (and not behind a hardware iommu)? If so, then yes it sounds like a
good application of multiple swiotlb pools.
> All this can then maybe be hidden behind the DMA API.
Won't we still need some changes to virtio to make use of its own pool (to
bounce buffers)? Something similar to its own DMA ops proposed in this patch?
> > +void virtio_bounce_set_dma_ops(struct virtio_device *vdev)
> > +{
> > + if (!bounce_buf_paddr)
> > + return;
> > +
> > + set_dma_ops(vdev->dev.parent, &virtio_dma_ops);
>
>
> I don't think DMA API maintainers will be happy with new users
> of set_dma_ops.
Is there an alternate API that is more preffered?
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, Apr 28, 2020 at 05:09:18PM +0530, Srivatsa Vaddagiri wrote:
> For better security, its desirable that a guest VM's memory is
> not accessible to any entity that executes outside the context of
> guest VM. In case of virtio, backend drivers execute outside the
> context of guest VM and in general will need access to complete
> guest VM memory. One option to restrict the access provided to
> backend driver is to make use of a bounce buffer. The bounce
> buffer is accessible to both backend and frontend drivers. All IO
> buffers that are in private space of guest VM are bounced to be
> accessible to backend.
>
> This patch proposes a new memory pool to be used for this bounce
> purpose, rather than the default swiotlb memory pool. That will
> avoid any conflicts that may arise in situations where a VM needs
> to use swiotlb pool for driving any pass-through devices (in
> which case swiotlb memory needs not be shared with another VM) as
> well as virtio devices (which will require swiotlb memory to be
> shared with backend VM). As a possible extension to this patch,
> we can provide an option for virtio to make use of default
> swiotlb memory pool itself, where no such conflicts may exist in
> a given deployment.
>
> Signed-off-by: Srivatsa Vaddagiri
Okay, but how is all this virtio specific? For example, why not allow
separate swiotlbs for any type of device?
For example, this might make sense if a given device is from a
different, less trusted vendor.
All this can then maybe be hidden behind the DMA API.
> ---
> drivers/virtio/Makefile| 2 +-
> drivers/virtio/virtio.c| 2 +
> drivers/virtio/virtio_bounce.c | 150
> +
> include/linux/virtio.h | 4 ++
> 4 files changed, 157 insertions(+), 1 deletion(-)
> create mode 100644 drivers/virtio/virtio_bounce.c
>
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 29a1386e..3fd3515 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_bounce.o
> obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32..bc2f779 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -329,6 +329,7 @@ int register_virtio_device(struct virtio_device *dev)
>
> dev->index = err;
> dev_set_name(&dev->dev, "virtio%u", dev->index);
> + virtio_bounce_set_dma_ops(dev);
>
> spin_lock_init(&dev->config_lock);
> dev->config_enabled = false;
> @@ -431,6 +432,7 @@ EXPORT_SYMBOL_GPL(virtio_device_restore);
>
> static int virtio_init(void)
> {
> + virtio_map_bounce_buffer();
> if (bus_register(&virtio_bus) != 0)
> panic("virtio bus registration failed");
> return 0;
> diff --git a/drivers/virtio/virtio_bounce.c b/drivers/virtio/virtio_bounce.c
> new file mode 100644
> index 000..3de8e0e
> --- /dev/null
> +++ b/drivers/virtio/virtio_bounce.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio DMA ops to bounce buffers
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + *
> + * This module allows bouncing of IO buffers to a region which will be
> + * accessible to backend drivers.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static phys_addr_t bounce_buf_paddr;
> +static void *bounce_buf_vaddr;
> +static size_t bounce_buf_size;
> +struct swiotlb_pool *virtio_pool;
> +
> +#define VIRTIO_MAX_BOUNCE_SIZE (16*4096)
> +
> +static void *virtio_alloc_coherent(struct device *dev, size_t size,
> + dma_addr_t *dma_handle, gfp_t gfp_flags, unsigned long attrs)
> +{
> + phys_addr_t addr;
> +
> + if (!virtio_pool)
> + return NULL;
> +
> + addr = swiotlb_alloc(virtio_pool, size, bounce_buf_paddr, ULONG_MAX);
> + if (addr == DMA_MAPPING_ERROR)
> + return NULL;
> +
> + *dma_handle = (addr - bounce_buf_paddr);
> +
> + return bounce_buf_vaddr + (addr - bounce_buf_paddr);
> +}
> +
> +static void virtio_free_coherent(struct device *dev, size_t size, void
> *vaddr,
> + dma_addr_t dma_handle, unsigned long attrs)
> +{
> + phys_addr_t addr = (dma_handle + bounce_buf_paddr);
> +
> + swiotlb_free(virtio_pool, addr, size);
> +}
> +
> +static dma_addr_t virtio_map_page(struct device *dev, struct page *page,
> + unsigned long offset, size_t size,
> + enum dma_data_direction dir, unsigned long attrs)
> +{
> + void *ptr = page_address(page) + offset;
> + phys_addr_t paddr = virt_to_phys(ptr);
> + dma_addr_t handle;
> +
> + if (!virtio_pool)
[PATCH 5/5] virtio: Add bounce DMA ops
For better security, its desirable that a guest VM's memory is
not accessible to any entity that executes outside the context of
guest VM. In case of virtio, backend drivers execute outside the
context of guest VM and in general will need access to complete
guest VM memory. One option to restrict the access provided to
backend driver is to make use of a bounce buffer. The bounce
buffer is accessible to both backend and frontend drivers. All IO
buffers that are in private space of guest VM are bounced to be
accessible to backend.
This patch proposes a new memory pool to be used for this bounce
purpose, rather than the default swiotlb memory pool. That will
avoid any conflicts that may arise in situations where a VM needs
to use swiotlb pool for driving any pass-through devices (in
which case swiotlb memory needs not be shared with another VM) as
well as virtio devices (which will require swiotlb memory to be
shared with backend VM). As a possible extension to this patch,
we can provide an option for virtio to make use of default
swiotlb memory pool itself, where no such conflicts may exist in
a given deployment.
Signed-off-by: Srivatsa Vaddagiri
---
drivers/virtio/Makefile| 2 +-
drivers/virtio/virtio.c| 2 +
drivers/virtio/virtio_bounce.c | 150 +
include/linux/virtio.h | 4 ++
4 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 drivers/virtio/virtio_bounce.c
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 29a1386e..3fd3515 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
+obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_bounce.o
obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32..bc2f779 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -329,6 +329,7 @@ int register_virtio_device(struct virtio_device *dev)
dev->index = err;
dev_set_name(&dev->dev, "virtio%u", dev->index);
+ virtio_bounce_set_dma_ops(dev);
spin_lock_init(&dev->config_lock);
dev->config_enabled = false;
@@ -431,6 +432,7 @@ EXPORT_SYMBOL_GPL(virtio_device_restore);
static int virtio_init(void)
{
+ virtio_map_bounce_buffer();
if (bus_register(&virtio_bus) != 0)
panic("virtio bus registration failed");
return 0;
diff --git a/drivers/virtio/virtio_bounce.c b/drivers/virtio/virtio_bounce.c
new file mode 100644
index 000..3de8e0e
--- /dev/null
+++ b/drivers/virtio/virtio_bounce.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio DMA ops to bounce buffers
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ *
+ * This module allows bouncing of IO buffers to a region which will be
+ * accessible to backend drivers.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+static phys_addr_t bounce_buf_paddr;
+static void *bounce_buf_vaddr;
+static size_t bounce_buf_size;
+struct swiotlb_pool *virtio_pool;
+
+#define VIRTIO_MAX_BOUNCE_SIZE (16*4096)
+
+static void *virtio_alloc_coherent(struct device *dev, size_t size,
+ dma_addr_t *dma_handle, gfp_t gfp_flags, unsigned long attrs)
+{
+ phys_addr_t addr;
+
+ if (!virtio_pool)
+ return NULL;
+
+ addr = swiotlb_alloc(virtio_pool, size, bounce_buf_paddr, ULONG_MAX);
+ if (addr == DMA_MAPPING_ERROR)
+ return NULL;
+
+ *dma_handle = (addr - bounce_buf_paddr);
+
+ return bounce_buf_vaddr + (addr - bounce_buf_paddr);
+}
+
+static void virtio_free_coherent(struct device *dev, size_t size, void *vaddr,
+ dma_addr_t dma_handle, unsigned long attrs)
+{
+ phys_addr_t addr = (dma_handle + bounce_buf_paddr);
+
+ swiotlb_free(virtio_pool, addr, size);
+}
+
+static dma_addr_t virtio_map_page(struct device *dev, struct page *page,
+ unsigned long offset, size_t size,
+ enum dma_data_direction dir, unsigned long attrs)
+{
+ void *ptr = page_address(page) + offset;
+ phys_addr_t paddr = virt_to_phys(ptr);
+ dma_addr_t handle;
+
+ if (!virtio_pool)
+ return DMA_MAPPING_ERROR;
+
+ handle = _swiotlb_tbl_map_single(virtio_pool, dev, bounce_buf_paddr,
+paddr, size, size, dir, attrs);
+ if (handle == (phys_addr_t)DMA_MAPPING_ERROR)
+ return DMA_MAPPING_ERROR;
+
+ return handle - bounce_buf_paddr;
+}
+
+static void virtio_unmap_page(struct device *dev, dma_addr_t dev_addr,
+ size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+ phys_addr_t addr = dev_addr + bounce_buf_pad
