* [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
* [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
* 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
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).