Messages in this thread |  | | Date | Sat, 23 Apr 2022 09:40:53 -0700 | From | Christoph Hellwig <> | Subject | Re: [PATCH V1 3/6] xen/virtio: Add option to restrict memory access under Xen |
| |
Please split this into one patch that creates grant-dma-ops, and another that sets up the virtio restricted access helpers.
> + > +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS > +int arch_has_restricted_virtio_memory_access(void) > +{ > + return (xen_has_restricted_virtio_memory_access() || > + cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)); > +}
So instead of hardcoding Xen here, this seems like a candidate for another cc_platform_has flag.
> +config XEN_VIRTIO > + bool "Xen virtio support" > + default n
n is the default default, so no need to specify it.
> +// SPDX-License-Identifier: GPL-2.0-only > +/******************************************************************************
The all * line is not the usual kernel style, I'd suggest to drop it.
> +static struct page *xen_grant_dma_alloc_pages(struct device *dev, size_t size, > + dma_addr_t *dma_handle, > + enum dma_data_direction dir, > + gfp_t gfp) > +{ > + WARN_ONCE(1, "xen_grant_dma_alloc_pages size %zu\n", size); > + return NULL; > +} > + > +static void xen_grant_dma_free_pages(struct device *dev, size_t size, > + struct page *vaddr, dma_addr_t dma_handle, > + enum dma_data_direction dir) > +{ > + WARN_ONCE(1, "xen_grant_dma_free_pages size %zu\n", size); > +}
Please just wire this up to the same implementation as .alloc and .free.
> + spin_lock(&xen_grant_dma_lock); > + list_add(&data->list, &xen_grant_dma_devices); > + spin_unlock(&xen_grant_dma_lock);
Hmm, having to do this device lookup for every DMA operation is going to suck. It might make sense to add a private field (e.g. as a union with the iommu field) in struct device instead.
But if not you probably want to switch to a more efficient data structure like the xarray at least.
> +EXPORT_SYMBOL_GPL(xen_grant_setup_dma_ops);
I don't think this has any modular users, or did I miss something?
|  |