Signed-off-by: Gerd Hoffmann <[email protected]>
---
samples/vfio-mdev/mbochs.c | 136 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 117 insertions(+), 19 deletions(-)
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 2535c3677c..ca7960adf5 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -71,11 +71,19 @@
#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 MBOCHS_EDID_BLOB_OFFSET (MBOCHS_EDID_SIZE/2)
#define STORE_LE16(addr, val) (*(u16 *)addr = val)
#define STORE_LE32(addr, val) (*(u32 *)addr = val)
@@ -95,16 +103,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 +131,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 +165,14 @@ 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_regs;
+ u8 edid_blob[0x400];
struct list_head dmabufs;
u32 active_id;
@@ -342,10 +364,20 @@ 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_regs;
+ if (edid->link_state != VFIO_DEVICE_GFX_LINK_STATE_UP ||
+ offset >= edid->edid_size) {
+ memset(buf, 0, count);
+ break;
+ }
+ memcpy(buf, mdev_state->edid_blob + offset, count);
+ break;
case 0x500 ... 0x515: /* bochs dispi interface */
if (count != 2)
goto unhandled;
@@ -365,6 +397,44 @@ static void handle_mmio_read(struct mdev_state *mdev_state, u16 offset,
}
}
+static void handle_edid_regs(struct mdev_state *mdev_state, u16 offset,
+ char *buf, u32 count, bool is_write)
+{
+ char *regs = (void *)&mdev_state->edid_regs;
+
+ if (offset + count > sizeof(mdev_state->edid_regs))
+ return;
+ if (count != 4)
+ return;
+ if (offset % 4)
+ return;
+
+ if (is_write) {
+ switch (offset) {
+ case offsetof(struct vfio_region_gfx_edid, link_state):
+ case offsetof(struct vfio_region_gfx_edid, edid_size):
+ memcpy(regs + offset, buf, count);
+ break;
+ default:
+ /* read-only regs */
+ break;
+ }
+ } else {
+ memcpy(buf, regs + offset, count);
+ }
+}
+
+static void handle_edid_blob(struct mdev_state *mdev_state, u16 offset,
+ char *buf, u32 count, bool is_write)
+{
+ if (offset + count > mdev_state->edid_regs.edid_max_size)
+ return;
+ if (is_write)
+ memcpy(mdev_state->edid_blob + offset, buf, count);
+ else
+ memcpy(buf, mdev_state->edid_blob + offset, count);
+}
+
static ssize_t mdev_access(struct mdev_device *mdev, char *buf, size_t count,
loff_t pos, bool is_write)
{
@@ -384,13 +454,25 @@ 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 (pos < MBOCHS_EDID_BLOB_OFFSET) {
+ handle_edid_regs(mdev_state, pos, buf, count, is_write);
+ } else {
+ pos -= MBOCHS_EDID_BLOB_OFFSET;
+ handle_edid_blob(mdev_state, pos, buf, count, is_write);
+ }
+
} else if (pos >= MBOCHS_MEMORY_BAR_OFFSET &&
pos + count <=
MBOCHS_MEMORY_BAR_OFFSET + mdev_state->memsize) {
@@ -471,6 +553,10 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)
mdev_state->next_id = 1;
mdev_state->type = type;
+ mdev_state->edid_regs.max_xres = type->max_x;
+ mdev_state->edid_regs.max_yres = type->max_y;
+ mdev_state->edid_regs.edid_offset = MBOCHS_EDID_BLOB_OFFSET;
+ mdev_state->edid_regs.edid_max_size = sizeof(mdev_state->edid_blob);
mbochs_create_config_space(mdev_state);
mbochs_reset(mdev);
@@ -932,16 +1018,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 +1050,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_GFX;
+ ext->type.subtype = VFIO_REGION_SUBTYPE_GFX_EDID;
+ break;
default:
region_info->size = 0;
region_info->offset = 0;
@@ -984,7 +1084,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 +1184,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 +1206,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 +1213,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 +1246,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
On 9/21/2018 2:00 PM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <[email protected]>
> ---
> samples/vfio-mdev/mbochs.c | 136 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 117 insertions(+), 19 deletions(-)
>
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index 2535c3677c..ca7960adf5 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -71,11 +71,19 @@
> #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 MBOCHS_EDID_BLOB_OFFSET (MBOCHS_EDID_SIZE/2)
>
> #define STORE_LE16(addr, val) (*(u16 *)addr = val)
> #define STORE_LE32(addr, val) (*(u32 *)addr = val)
> @@ -95,16 +103,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 +131,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 +165,14 @@ 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_regs;
> + u8 edid_blob[0x400];
>
> struct list_head dmabufs;
> u32 active_id;
> @@ -342,10 +364,20 @@ 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_regs;
> + if (edid->link_state != VFIO_DEVICE_GFX_LINK_STATE_UP ||
> + offset >= edid->edid_size) {
> + memset(buf, 0, count);
> + break;
> + }
> + memcpy(buf, mdev_state->edid_blob + offset, count);
> + break;
> case 0x500 ... 0x515: /* bochs dispi interface */
> if (count != 2)
> goto unhandled;
> @@ -365,6 +397,44 @@ static void handle_mmio_read(struct mdev_state *mdev_state, u16 offset,
> }
> }
>
> +static void handle_edid_regs(struct mdev_state *mdev_state, u16 offset,
> + char *buf, u32 count, bool is_write)
> +{
> + char *regs = (void *)&mdev_state->edid_regs;
> +
> + if (offset + count > sizeof(mdev_state->edid_regs))
> + return;
> + if (count != 4)
> + return;
> + if (offset % 4)
> + return;
> +
> + if (is_write) {
> + switch (offset) {
> + case offsetof(struct vfio_region_gfx_edid, link_state):
> + case offsetof(struct vfio_region_gfx_edid, edid_size):
> + memcpy(regs + offset, buf, count);
> + break;
> + default:
> + /* read-only regs */
> + break;
> + }
> + } else {
> + memcpy(buf, regs + offset, count);
> + }
> +}
> +
> +static void handle_edid_blob(struct mdev_state *mdev_state, u16 offset,
> + char *buf, u32 count, bool is_write)
> +{
> + if (offset + count > mdev_state->edid_regs.edid_max_size)
> + return;
> + if (is_write)
> + memcpy(mdev_state->edid_blob + offset, buf, count);
> + else
> + memcpy(buf, mdev_state->edid_blob + offset, count);
> +}
> +
> static ssize_t mdev_access(struct mdev_device *mdev, char *buf, size_t count,
> loff_t pos, bool is_write)
> {
> @@ -384,13 +454,25 @@ 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 (pos < MBOCHS_EDID_BLOB_OFFSET) {
> + handle_edid_regs(mdev_state, pos, buf, count, is_write);
> + } else {
> + pos -= MBOCHS_EDID_BLOB_OFFSET;
> + handle_edid_blob(mdev_state, pos, buf, count, is_write);
> + }
> +
> } else if (pos >= MBOCHS_MEMORY_BAR_OFFSET &&
> pos + count <=
> MBOCHS_MEMORY_BAR_OFFSET + mdev_state->memsize) {
> @@ -471,6 +553,10 @@ static int mbochs_create(struct kobject *kobj, struct mdev_device *mdev)
> mdev_state->next_id = 1;
>
> mdev_state->type = type;
> + mdev_state->edid_regs.max_xres = type->max_x;
> + mdev_state->edid_regs.max_yres = type->max_y;
> + mdev_state->edid_regs.edid_offset = MBOCHS_EDID_BLOB_OFFSET;
> + mdev_state->edid_regs.edid_max_size = sizeof(mdev_state->edid_blob);
> mbochs_create_config_space(mdev_state);
> mbochs_reset(mdev);
>
> @@ -932,16 +1018,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 +1050,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);
Any reason to not to use _MMAP flag?
How would QEMU side code read this region? will it be always trapped?
If vendor driver sets _MMAP flag, will QEMU side handle that case as well?
I think since its blob, edid could be read by QEMU using one memcpy
rather than adding multiple memcpy of 4 or 8 bytes.
Thanks,
Kirti
> + 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_GFX;
> + ext->type.subtype = VFIO_REGION_SUBTYPE_GFX_EDID;
> + break;
> default:
> region_info->size = 0;
> region_info->offset = 0;
> @@ -984,7 +1084,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 +1184,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 +1206,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 +1213,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 +1246,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);
>
On Fri, 28 Sep 2018 01:27:16 +0530
Kirti Wankhede <[email protected]> wrote:
> On 9/21/2018 2:00 PM, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <[email protected]>
> > @@ -964,6 +1050,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);
>
> Any reason to not to use _MMAP flag?
> How would QEMU side code read this region? will it be always trapped?
> If vendor driver sets _MMAP flag, will QEMU side handle that case as well?
> I think since its blob, edid could be read by QEMU using one memcpy
> rather than adding multiple memcpy of 4 or 8 bytes.
"Trapping" would only come into play if the region were exposed to the
VM, which there's no intention to do here afaik. Also, just because
it doesn't support mmap doesn't mean that QEMU necessarily needs to
break down accesses into smaller words, QEMU could do:
pwrite(fd, buf, edid_max_size, region_offset + edid_offset)
ie. write the entire edid area with one operation. I don't think
there's anything in the specification that prevents mmap now,
edid_offset could be at a page alignment, edid_max_size could be
PAGE_SIZE, and a sparse mmap capability could indicate that only the
EDID area is mmap'able, but is it worth the code to support that?
Thanks,
Alex
> > + 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);
>
> Any reason to not to use _MMAP flag?
There is no page backing this. Also it is not performance-critical,
edid updates should be rare, so the extra code for mmap support doesn't
look like it is worth it.
Also for the virtual registers (especially link_state) it is probably
useful to have the write callback of the mdev driver called to get
notified about the change.
> How would QEMU side code read this region? will it be always trapped?
qemu uses read & write syscalls (well, pread & pwrite actually).
> If vendor driver sets _MMAP flag, will QEMU side handle that case as well?
The current test branch doesn't, it expects read+write to work.
https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid-vfio
> I think since its blob, edid could be read by QEMU using one memcpy
> rather than adding multiple memcpy of 4 or 8 bytes.
From qemu it's a single pwrite syscall actually. mbochs_write() splits
it into 4 byte writes and calls mbochs_access() for each of them. One
could probably add a special case for the EDID blob to mbochs_write().
But again: doesn't seem worth the effort given that edid updates should
be a rare event.
cheers,
Gerd
On 9/28/2018 11:10 AM, Gerd Hoffmann wrote:
>>> + 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);
>>
>> Any reason to not to use _MMAP flag?
>
> There is no page backing this. Also it is not performance-critical,
> edid updates should be rare, so the extra code for mmap support doesn't
> look like it is worth it.
>
> Also for the virtual registers (especially link_state) it is probably
> useful to have the write callback of the mdev driver called to get
> notified about the change.
>
>> How would QEMU side code read this region? will it be always trapped?
>
> qemu uses read & write syscalls (well, pread & pwrite actually).
>
>> If vendor driver sets _MMAP flag, will QEMU side handle that case as well?
>
> The current test branch doesn't, it expects read+write to work.
> https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid-vfio
>
Ok.
Can you add a comment in vfio.h that this region is non-mmappable?
>> I think since its blob, edid could be read by QEMU using one memcpy
>> rather than adding multiple memcpy of 4 or 8 bytes.
>
> From qemu it's a single pwrite syscall actually. mbochs_write() splits
> it into 4 byte writes and calls mbochs_access() for each of them. One
> could probably add a special case for the EDID blob to mbochs_write().
> But again: doesn't seem worth the effort given that edid updates should
> be a rare event.
>
Ok.
Thanks,
Kirti
> cheers,
> Gerd
>
On Fri, 28 Sep 2018 14:10:26 +0530
Kirti Wankhede <[email protected]> wrote:
> On 9/28/2018 11:10 AM, Gerd Hoffmann wrote:
> >>> + 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);
> >>
> >> Any reason to not to use _MMAP flag?
> >
> > There is no page backing this. Also it is not performance-critical,
> > edid updates should be rare, so the extra code for mmap support doesn't
> > look like it is worth it.
> >
> > Also for the virtual registers (especially link_state) it is probably
> > useful to have the write callback of the mdev driver called to get
> > notified about the change.
> >
> >> How would QEMU side code read this region? will it be always trapped?
> >
> > qemu uses read & write syscalls (well, pread & pwrite actually).
> >
> >> If vendor driver sets _MMAP flag, will QEMU side handle that case as well?
> >
> > The current test branch doesn't, it expects read+write to work.
> > https://git.kraxel.org/cgit/qemu/log/?h=sirius/edid-vfio
> >
>
> Ok.
> Can you add a comment in vfio.h that this region is non-mmappable?
No, region access mechanisms are self describing through the region
info ioctl, it's left to the implementation to decide what to support.
The region definition should not impose such a restriction. Thanks,
Alex