* [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices. [not found] <20180918133813.1845-1-kraxel@redhat.com> @ 2018-09-18 13:38 ` Gerd Hoffmann 2018-09-19 19:52 ` Alex Williamson 2018-09-18 13:38 ` [PATCH v2 2/2] vfio: add edid support to mbochs sample driver Gerd Hoffmann 1 sibling, 1 reply; 4+ messages in thread From: Gerd Hoffmann @ 2018-09-18 13:38 UTC (permalink / raw) To: Kirti Wankhede, intel-gvt-dev, Alex Williamson Cc: kvm, Gerd Hoffmann, open list Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/uapi/linux/vfio.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 1aa7b82e81..78e5a37d83 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -301,6 +301,45 @@ struct vfio_region_info_cap_type { #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2) #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3) +#define VFIO_REGION_TYPE_PCI_GFX (1) +#define VFIO_REGION_SUBTYPE_GFX_EDID (1) + +/** + * Set display link state and edid blob. + * + * For the edid blob spec look here: + * https://en.wikipedia.org/wiki/Extended_Display_Identification_Data + * + * The guest should be notified about edid changes, for example by + * setting the link status to down temporarely (emulate monitor + * hotplug). + * + * @link_state: + * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on. + * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off. + * + * @edid_size: Size of the edid data blob. + * @edid_blob: The actual edid data. + * + * Returns 0 on success, error code (such as -EINVAL) on failure. + */ +struct vfio_region_gfx_edid { + /* device capability hints (read only) */ + __u32 max_xres; + __u32 max_yres; + __u32 __reserved1[6]; + + /* device state (read/write) */ + __u32 link_state; +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 + __u32 edid_size; + __u32 __reserved2[6]; + + /* edid blob (read/write) */ + __u8 edid_blob[512]; +}; + /* * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped * which allows direct access to non-MSIX registers which happened to be within -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices. 2018-09-18 13:38 ` [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices Gerd Hoffmann @ 2018-09-19 19:52 ` Alex Williamson 2018-09-20 8:11 ` Gerd Hoffmann 0 siblings, 1 reply; 4+ messages in thread From: Alex Williamson @ 2018-09-19 19:52 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: Kirti Wankhede, intel-gvt-dev, kvm, open list On Tue, 18 Sep 2018 15:38:12 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: No empty commit logs please. There must be something to say about the goal or motivation beyond the subject. > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/uapi/linux/vfio.h | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 1aa7b82e81..78e5a37d83 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -301,6 +301,45 @@ struct vfio_region_info_cap_type { > #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2) > #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3) > > +#define VFIO_REGION_TYPE_PCI_GFX (1) nit, what's the PCI dependency? > +#define VFIO_REGION_SUBTYPE_GFX_EDID (1) > + > +/** > + * Set display link state and edid blob. > + * > + * For the edid blob spec look here: > + * https://en.wikipedia.org/wiki/Extended_Display_Identification_Data > + * > + * The guest should be notified about edid changes, for example by > + * setting the link status to down temporarely (emulate monitor > + * hotplug). Who is responsible for this notification, the user interacting with this region or the driver providing the region when a new edid is provided? This comment needs to state the expected API as clearly as possible. > + * > + * @link_state: > + * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on. > + * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off. > + * > + * @edid_size: Size of the edid data blob. > + * @edid_blob: The actual edid data. What signals that the user edid_blob update is complete? Should the size be written before or after the blob? Is the user required to update the entire blob in a single write or can it be written incrementally? It might also be worth defining access widths, I see that you use memcpy to support any width in mbochs, but we could define only native field accesses for discrete registers if it makes the implementation easier. > + * > + * Returns 0 on success, error code (such as -EINVAL) on failure. Left over from ioctl. > + */ > +struct vfio_region_gfx_edid { > + /* device capability hints (read only) */ > + __u32 max_xres; > + __u32 max_yres; > + __u32 __reserved1[6]; Is the plan to use the version field within vfio_info_cap_header to make use of these reserved fields later, ie. version 2 might define a field from this reserved block? > + > + /* device state (read/write) */ > + __u32 link_state; > +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 > +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 > + __u32 edid_size; > + __u32 __reserved2[6]; > + > + /* edid blob (read/write) */ > + __u8 edid_blob[512]; It seems the placement of this blob is what makes us feel like we need to introduce reserved fields for later use, but we could instead define an edid_offset read-only field so that the blob is always at the end of whatever discrete fields we define. Perhaps then we wouldn't even need a read-only vs read-write section, simply define it per virtual register. Overall, I prefer this approach rather than adding yet more ioctls for every feature and extension we add, thanks for implementing it. What's your impression vs ioctls? Thanks, Alex ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices. 2018-09-19 19:52 ` Alex Williamson @ 2018-09-20 8:11 ` Gerd Hoffmann 0 siblings, 0 replies; 4+ messages in thread From: Gerd Hoffmann @ 2018-09-20 8:11 UTC (permalink / raw) To: Alex Williamson; +Cc: Kirti Wankhede, intel-gvt-dev, kvm, open list On Wed, Sep 19, 2018 at 01:52:19PM -0600, Alex Williamson wrote: > On Tue, 18 Sep 2018 15:38:12 +0200 > Gerd Hoffmann <kraxel@redhat.com> wrote: > > No empty commit logs please. There must be something to say about the > goal or motivation beyond the subject. > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > include/uapi/linux/vfio.h | 39 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > index 1aa7b82e81..78e5a37d83 100644 > > --- a/include/uapi/linux/vfio.h > > +++ b/include/uapi/linux/vfio.h > > @@ -301,6 +301,45 @@ struct vfio_region_info_cap_type { > > #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2) > > #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3) > > > > +#define VFIO_REGION_TYPE_PCI_GFX (1) > > nit, what's the PCI dependency? None I guess, just copy&paste from the (pci) vendor cap above ... Will rename it. > > + * The guest should be notified about edid changes, for example by > > + * setting the link status to down temporarely (emulate monitor > > + * hotplug). > > Who is responsible for this notification, the user interacting with > this region or the driver providing the region when a new edid is > provided? This comment needs to state the expected API as clearly as > possible. > > + * @link_state: > > + * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on. > > + * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off. > > + * > > + * @edid_size: Size of the edid data blob. > > + * @edid_blob: The actual edid data. > > What signals that the user edid_blob update is complete? Should the > size be written before or after the blob? Is the user required to > update the entire blob in a single write or can it be written > incrementally? My qemu test code first sets link-state = down, then updates edid, then sets link-state = up. I can document that as official update protocol. Drivers should be able to map it do emulation events pretty straight forward. Except maybe for the final down-up transition, drivers might have to delay this to make sure the guest has seen the up-down transition. > It might also be worth defining access widths, I see that you use > memcpy to support any width in mbochs, but we could define only native > field accesses for discrete registers if it makes the implementation > easier. Makes sense for the u32 registers (especially the writable ones). For mbochs the virtual hardware has no link state notification, so I can just store whatever I get, then check link-state when the guest attempts to read the edid. When actually implementing link-state notification it is probably useful to not have to deal with a half-written link-state field ... > > + */ > > +struct vfio_region_gfx_edid { > > + /* device capability hints (read only) */ > > + __u32 max_xres; > > + __u32 max_yres; > > + __u32 __reserved1[6]; > > Is the plan to use the version field within vfio_info_cap_header to > make use of these reserved fields later, ie. version 2 might define a > field from this reserved block? Yes. > > + /* device state (read/write) */ > > + __u32 link_state; > > +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1 > > +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 > > + __u32 edid_size; > > + __u32 __reserved2[6]; > > + > > + /* edid blob (read/write) */ > > + __u8 edid_blob[512]; > > It seems the placement of this blob is what makes us feel like we need > to introduce reserved fields for later use, but we could instead define > an edid_offset read-only field so that the blob is always at the end of > whatever discrete fields we define. Yes, sounds sensible. > Perhaps then we wouldn't even need a read-only vs read-write section, > simply define it per virtual register. I'll look into this. > Overall, I prefer this approach rather than adding yet more ioctls for > every feature and extension we add, thanks for implementing it. What's > your impression vs ioctls? I'd still prefer ioctls, but this is workable too. So if you like it better this way lets go for it. cheers, Gerd ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 2/2] vfio: add edid support to mbochs sample driver [not found] <20180918133813.1845-1-kraxel@redhat.com> 2018-09-18 13:38 ` [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices Gerd Hoffmann @ 2018-09-18 13:38 ` Gerd Hoffmann 1 sibling, 0 replies; 4+ messages in thread From: Gerd Hoffmann @ 2018-09-18 13:38 UTC (permalink / raw) To: Kirti Wankhede, intel-gvt-dev, Alex Williamson Cc: kvm, Gerd Hoffmann, open list Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- samples/vfio-mdev/mbochs.c | 115 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 96 insertions(+), 19 deletions(-) diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index 2535c3677c..4e72d36ff3 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -71,11 +71,17 @@ #define MBOCHS_NAME "mbochs" #define MBOCHS_CLASS_NAME "mbochs" +#define MBOCHS_EDID_REGION_INDEX VFIO_PCI_NUM_REGIONS +#define MBOCHS_NUM_REGIONS (MBOCHS_EDID_REGION_INDEX+1) + #define MBOCHS_CONFIG_SPACE_SIZE 0xff #define MBOCHS_MMIO_BAR_OFFSET PAGE_SIZE #define MBOCHS_MMIO_BAR_SIZE PAGE_SIZE -#define MBOCHS_MEMORY_BAR_OFFSET (MBOCHS_MMIO_BAR_OFFSET + \ +#define MBOCHS_EDID_OFFSET (MBOCHS_MMIO_BAR_OFFSET + \ MBOCHS_MMIO_BAR_SIZE) +#define MBOCHS_EDID_SIZE PAGE_SIZE +#define MBOCHS_MEMORY_BAR_OFFSET (MBOCHS_EDID_OFFSET + \ + MBOCHS_EDID_SIZE) #define STORE_LE16(addr, val) (*(u16 *)addr = val) #define STORE_LE32(addr, val) (*(u32 *)addr = val) @@ -95,16 +101,24 @@ MODULE_PARM_DESC(mem, "megabytes available to " MBOCHS_NAME " devices"); static const struct mbochs_type { const char *name; u32 mbytes; + u32 max_x; + u32 max_y; } mbochs_types[] = { { .name = MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_1, .mbytes = 4, + .max_x = 800, + .max_y = 600, }, { .name = MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_2, .mbytes = 16, + .max_x = 1920, + .max_y = 1440, }, { .name = MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_3, .mbytes = 64, + .max_x = 0, + .max_y = 0, }, }; @@ -115,6 +129,11 @@ static struct cdev mbochs_cdev; static struct device mbochs_dev; static int mbochs_used_mbytes; +struct vfio_region_info_ext { + struct vfio_region_info base; + struct vfio_region_info_cap_type type; +}; + struct mbochs_mode { u32 drm_format; u32 bytepp; @@ -144,13 +163,13 @@ struct mdev_state { u32 memory_bar_mask; struct mutex ops_lock; struct mdev_device *mdev; - struct vfio_device_info dev_info; const struct mbochs_type *type; u16 vbe[VBE_DISPI_INDEX_COUNT]; u64 memsize; struct page **pages; pgoff_t pagecount; + struct vfio_region_gfx_edid edid_region; struct list_head dmabufs; u32 active_id; @@ -342,10 +361,22 @@ static void handle_mmio_read(struct mdev_state *mdev_state, u16 offset, char *buf, u32 count) { struct device *dev = mdev_dev(mdev_state->mdev); + struct vfio_region_gfx_edid *edid; u16 reg16 = 0; int index; switch (offset) { + case 0x000 ... 0x3ff: /* edid block */ + edid = &mdev_state->edid_region; + if (offset + count > sizeof(edid->edid_blob)) + goto unhandled; + if (edid->link_state != VFIO_DEVICE_GFX_LINK_STATE_UP || + offset >= edid->edid_size) { + memset(buf, 0, count); + break; + } + memcpy(buf, edid->edid_blob + offset, count); + break; case 0x500 ... 0x515: /* bochs dispi interface */ if (count != 2) goto unhandled; @@ -365,6 +396,28 @@ static void handle_mmio_read(struct mdev_state *mdev_state, u16 offset, } } +static void handle_edid_write(struct mdev_state *mdev_state, u16 offset, + char *buf, u32 count) +{ + char *region = (void *)&mdev_state->edid_region; + + if (offset < offsetof(typeof(mdev_state->edid_region), link_state)) + return; + if (offset + count > sizeof(mdev_state->edid_region)) + return; + memcpy(region + offset, buf, count); +} + +static void handle_edid_read(struct mdev_state *mdev_state, u16 offset, + char *buf, u32 count) +{ + char *region = (void *)&mdev_state->edid_region; + + if (offset + count > sizeof(mdev_state->edid_region)) + return; + memcpy(buf, region + offset, count); +} + static ssize_t mdev_access(struct mdev_device *mdev, char *buf, size_t count, loff_t pos, bool is_write) { @@ -384,13 +437,23 @@ static ssize_t mdev_access(struct mdev_device *mdev, char *buf, size_t count, memcpy(buf, (mdev_state->vconfig + pos), count); } else if (pos >= MBOCHS_MMIO_BAR_OFFSET && - pos + count <= MBOCHS_MEMORY_BAR_OFFSET) { + pos + count <= (MBOCHS_MMIO_BAR_OFFSET + + MBOCHS_MMIO_BAR_SIZE)) { pos -= MBOCHS_MMIO_BAR_OFFSET; if (is_write) handle_mmio_write(mdev_state, pos, buf, count); else handle_mmio_read(mdev_state, pos, buf, count); + } else if (pos >= MBOCHS_EDID_OFFSET && + pos + count <= (MBOCHS_EDID_OFFSET + + MBOCHS_EDID_SIZE)) { + pos -= MBOCHS_EDID_OFFSET; + if (is_write) + handle_edid_write(mdev_state, pos, buf, count); + else + handle_edid_read(mdev_state, pos, buf, count); + } else if (pos >= MBOCHS_MEMORY_BAR_OFFSET && pos + count <= MBOCHS_MEMORY_BAR_OFFSET + mdev_state->memsize) { @@ -471,6 +534,8 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev) mdev_state->next_id = 1; mdev_state->type = type; + mdev_state->edid_region.max_xres = type->max_x; + mdev_state->edid_region.max_yres = type->max_y; mbochs_create_config_space(mdev_state); mbochs_reset(mdev); @@ -932,16 +997,16 @@ static int mbochs_dmabuf_export(struct mbochs_dmabuf *dmabuf) } static int mbochs_get_region_info(struct mdev_device *mdev, - struct vfio_region_info *region_info, - u16 *cap_type_id, void **cap_type) + struct vfio_region_info_ext *ext) { + struct vfio_region_info *region_info = &ext->base; struct mdev_state *mdev_state; mdev_state = mdev_get_drvdata(mdev); if (!mdev_state) return -EINVAL; - if (region_info->index >= VFIO_PCI_NUM_REGIONS) + if (region_info->index >= MBOCHS_NUM_REGIONS) return -EINVAL; switch (region_info->index) { @@ -964,6 +1029,20 @@ static int mbochs_get_region_info(struct mdev_device *mdev, region_info->flags = (VFIO_REGION_INFO_FLAG_READ | VFIO_REGION_INFO_FLAG_WRITE); break; + case MBOCHS_EDID_REGION_INDEX: + ext->base.argsz = sizeof(*ext); + ext->base.offset = MBOCHS_EDID_OFFSET; + ext->base.size = MBOCHS_EDID_SIZE; + ext->base.flags = (VFIO_REGION_INFO_FLAG_READ | + VFIO_REGION_INFO_FLAG_WRITE | + VFIO_REGION_INFO_FLAG_CAPS); + ext->base.cap_offset = offsetof(typeof(*ext), type); + ext->type.header.id = VFIO_REGION_INFO_CAP_TYPE; + ext->type.header.version = 1; + ext->type.header.next = 0; + ext->type.type = VFIO_REGION_TYPE_PCI_GFX; + ext->type.subtype = VFIO_REGION_SUBTYPE_GFX_EDID; + break; default: region_info->size = 0; region_info->offset = 0; @@ -984,7 +1063,7 @@ static int mbochs_get_device_info(struct mdev_device *mdev, struct vfio_device_info *dev_info) { dev_info->flags = VFIO_DEVICE_FLAGS_PCI; - dev_info->num_regions = VFIO_PCI_NUM_REGIONS; + dev_info->num_regions = MBOCHS_NUM_REGIONS; dev_info->num_irqs = VFIO_PCI_NUM_IRQS; return 0; } @@ -1084,7 +1163,7 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd, unsigned long arg) { int ret = 0; - unsigned long minsz; + unsigned long minsz, outsz; struct mdev_state *mdev_state; mdev_state = mdev_get_drvdata(mdev); @@ -1106,8 +1185,6 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd, if (ret) return ret; - memcpy(&mdev_state->dev_info, &info, sizeof(info)); - if (copy_to_user((void __user *)arg, &info, minsz)) return -EFAULT; @@ -1115,24 +1192,24 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd, } case VFIO_DEVICE_GET_REGION_INFO: { - struct vfio_region_info info; - u16 cap_type_id = 0; - void *cap_type = NULL; + struct vfio_region_info_ext info; - minsz = offsetofend(struct vfio_region_info, offset); + minsz = offsetofend(typeof(info), base.offset); if (copy_from_user(&info, (void __user *)arg, minsz)) return -EFAULT; - if (info.argsz < minsz) + outsz = info.base.argsz; + if (outsz < minsz) + return -EINVAL; + if (outsz > sizeof(info)) return -EINVAL; - ret = mbochs_get_region_info(mdev, &info, &cap_type_id, - &cap_type); + ret = mbochs_get_region_info(mdev, &info); if (ret) return ret; - if (copy_to_user((void __user *)arg, &info, minsz)) + if (copy_to_user((void __user *)arg, &info, outsz)) return -EFAULT; return 0; @@ -1148,7 +1225,7 @@ static long mbochs_ioctl(struct mdev_device *mdev, unsigned int cmd, return -EFAULT; if ((info.argsz < minsz) || - (info.index >= mdev_state->dev_info.num_irqs)) + (info.index >= VFIO_PCI_NUM_IRQS)) return -EINVAL; ret = mbochs_get_irq_info(mdev, &info); -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-20 8:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20180918133813.1845-1-kraxel@redhat.com> 2018-09-18 13:38 ` [PATCH v2 1/2] vfio: add edid api for display (vgpu) devices Gerd Hoffmann 2018-09-19 19:52 ` Alex Williamson 2018-09-20 8:11 ` Gerd Hoffmann 2018-09-18 13:38 ` [PATCH v2 2/2] vfio: add edid support to mbochs sample driver Gerd Hoffmann
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.