| Message ID | 20190801022517.1903-3-gurchetansingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | udmabuf guest <--> host interaction model | expand |
On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote: > The main use for udmabuf is sending guest memory pages > to the host. > > It's generally a bad idea to have to separate mappings with > different attributes. For example, a WC mapping the guest > kernel and cached mapping on the host is problematic. > > Add creation time flags so the user has responsibility for > the specific use case. > -#define UDMABUF_FLAGS_CLOEXEC 0x01 > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > +#define UDMABUF_FLAGS_PROT_NONE 0x02 > +#define UDMABUF_FLAGS_PROT_READ 0x04 > +#define UDMABUF_FLAGS_PROT_WRITE 0x08 [ didn't look at followup patches yet ] You can't have readonly/writeonly dmabufs. So that isn't going to fly. The commit message suggests this is for cache attributes not protection, so having the flags might make sense, but please don't name the flags PROT_* then. cheers, Gerd
On Wed, Jul 31, 2019 at 11:40 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote: > > The main use for udmabuf is sending guest memory pages > > to the host. > > > > It's generally a bad idea to have to separate mappings with > > different attributes. For example, a WC mapping the guest > > kernel and cached mapping on the host is problematic. > > > > Add creation time flags so the user has responsibility for > > the specific use case. > > > -#define UDMABUF_FLAGS_CLOEXEC 0x01 > > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > > +#define UDMABUF_FLAGS_PROT_NONE 0x02 > > +#define UDMABUF_FLAGS_PROT_READ 0x04 > > +#define UDMABUF_FLAGS_PROT_WRITE 0x08 > > [ didn't look at followup patches yet ] > > You can't have readonly/writeonly dmabufs. > So that isn't going to fly. > > The commit message suggests this is for cache attributes not protection, > so having the flags might make sense, but please don't name the flags > PROT_* then. Okay, I'll change the flags to CACHED / UNCACHED / WRITE_COMBINE (like msm_drm.h). And since the dma api doesn't work on x86 [1], we'll have to call drm_cflush_pages in the guest. Since caching is privileged on ARM and not on x86, that *should* get us write-combine guest buffers. [1] https://lists.freedesktop.org/archives/dri-devel/2019-August/229161.html > > cheers, > Gerd >
On Fri, Aug 02, 2019 at 09:45:15AM -0700, Gurchetan Singh wrote: > On Wed, Jul 31, 2019 at 11:40 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote: > > > The main use for udmabuf is sending guest memory pages > > > to the host. > > > > > > It's generally a bad idea to have to separate mappings with > > > different attributes. For example, a WC mapping the guest > > > kernel and cached mapping on the host is problematic. > > > > > > Add creation time flags so the user has responsibility for > > > the specific use case. > > > > > -#define UDMABUF_FLAGS_CLOEXEC 0x01 > > > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > > > +#define UDMABUF_FLAGS_PROT_NONE 0x02 > > > +#define UDMABUF_FLAGS_PROT_READ 0x04 > > > +#define UDMABUF_FLAGS_PROT_WRITE 0x08 > > > > [ didn't look at followup patches yet ] > > > > You can't have readonly/writeonly dmabufs. > > So that isn't going to fly. > > > > The commit message suggests this is for cache attributes not protection, > > so having the flags might make sense, but please don't name the flags > > PROT_* then. > > Okay, I'll change the flags to CACHED / UNCACHED / WRITE_COMBINE (like > msm_drm.h). And since the dma api doesn't work on x86 [1], we'll have > to call drm_cflush_pages in the guest. Since caching is privileged on > ARM and not on x86, that *should* get us write-combine guest buffers. > > [1] https://lists.freedesktop.org/archives/dri-devel/2019-August/229161.html Ah, so you are aware of the vgem cache synchronization patches. It's probably a good idea to wait until that is finally settled before following with udmabuf. cheers, Gerd
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index b345e91d831a..4ecf2a94fed3 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -186,7 +186,12 @@ static long udmabuf_create(const struct udmabuf_create_list *head, exp_info.ops = &udmabuf_ops; exp_info.size = ubuf->pagecount << PAGE_SHIFT; exp_info.priv = ubuf; - exp_info.flags = O_RDWR; + + if ((head->flags & UDMABUF_FLAGS_PROT_READ) && + (head->flags & UDMABUF_FLAGS_PROT_WRITE)) + exp_info.flags = O_RDWR; + else if (head->flags & UDMABUF_FLAGS_PROT_WRITE) + exp_info.flags = O_WRONLY; buf = dma_buf_export(&exp_info); if (IS_ERR(buf)) { diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h index 46b6532ed855..21290cb9696c 100644 --- a/include/uapi/linux/udmabuf.h +++ b/include/uapi/linux/udmabuf.h @@ -5,7 +5,10 @@ #include <linux/types.h> #include <linux/ioctl.h> -#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_CLOEXEC 0x01 +#define UDMABUF_FLAGS_PROT_NONE 0x02 +#define UDMABUF_FLAGS_PROT_READ 0x04 +#define UDMABUF_FLAGS_PROT_WRITE 0x08 struct udmabuf_create { __u32 memfd;
The main use for udmabuf is sending guest memory pages to the host. It's generally a bad idea to have to separate mappings with different attributes. For example, a WC mapping the guest kernel and cached mapping on the host is problematic. Add creation time flags so the user has responsibility for the specific use case. Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> --- drivers/dma-buf/udmabuf.c | 7 ++++++- include/uapi/linux/udmabuf.h | 5 ++++- 2 files changed, 10 insertions(+), 2 deletions(-)