[v2,3/8] xen, gfx passthrough: basic graphics passthrough support

basic gfx passthrough support:
- add a vga type for gfx passthrough
- retrieve VGA bios from sysfs, then load it to guest 0xC0000
- register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx

The original patch is from Weidong Han <weidong.han@intel.com>

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Cc: Weidong Han <weidong.han@intel.com>
---
v2:

* retrieve VGA bios from sysfs properly.
* redefine some function name.

 hw/xen/Makefile.objs         |   2 +-
 hw/xen/xen-host-pci-device.c |   5 ++
 hw/xen/xen-host-pci-device.h |   1 +
 hw/xen/xen_pt.c              |  10 +++
 hw/xen/xen_pt.h              |   4 +
 hw/xen/xen_pt_graphics.c     | 177 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx              |   9 +++
 vl.c                         |  11 ++-
 8 files changed, 217 insertions(+), 2 deletions(-)
 create mode 100644 hw/xen/xen_pt_graphics.c

On Fri, May 16, 2014 at 06:53:39PM +0800, Tiejun Chen wrote:
> basic gfx passthrough support:
> - add a vga type for gfx passthrough
> - retrieve VGA bios from sysfs, then load it to guest 0xC0000
> - register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx
> 
> The original patch is from Weidong Han <weidong.han@intel.com>
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> Cc: Weidong Han <weidong.han@intel.com>
> ---
> v2:
> 
> * retrieve VGA bios from sysfs properly.
> * redefine some function name.
> 
>  hw/xen/Makefile.objs         |   2 +-
>  hw/xen/xen-host-pci-device.c |   5 ++
>  hw/xen/xen-host-pci-device.h |   1 +
>  hw/xen/xen_pt.c              |  10 +++
>  hw/xen/xen_pt.h              |   4 +
>  hw/xen/xen_pt_graphics.c     | 177 +++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx              |   9 +++
>  vl.c                         |  11 ++-
>  8 files changed, 217 insertions(+), 2 deletions(-)
>  create mode 100644 hw/xen/xen_pt_graphics.c
> 
> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> index a0ca0aa..77b7dae 100644
> --- a/hw/xen/Makefile.objs
> +++ b/hw/xen/Makefile.objs
> @@ -2,4 +2,4 @@
>  common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
>  
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> -obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> +obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o xen_pt_graphics.o
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 743b37b..a54b7de 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -376,6 +376,11 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>          goto error;
>      }
>      d->irq = v;
> +    rc = xen_host_pci_get_hex_value(d, "class", &v);
> +    if (rc) {
> +        goto error;
> +    }
> +    d->class_code = v;
>      d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
>  
>      return 0;
> diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> index c2486f0..f1e1c30 100644
> --- a/hw/xen/xen-host-pci-device.h
> +++ b/hw/xen/xen-host-pci-device.h
> @@ -25,6 +25,7 @@ typedef struct XenHostPCIDevice {
>  
>      uint16_t vendor_id;
>      uint16_t device_id;
> +    uint32_t class_code;
>      int irq;
>  
>      XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1];
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index be4220b..a0113ea 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -450,6 +450,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
>                     d->rom.size, d->rom.base_addr);
>      }
>  
> +    xen_pt_register_vga_regions(d);
>      return 0;
>  }
>  
> @@ -470,6 +471,8 @@ static void xen_pt_unregister_regions(XenPCIPassthroughState *s)
>      if (d->rom.base_addr && d->rom.size) {
>          memory_region_destroy(&s->rom);
>      }
> +
> +    xen_pt_unregister_vga_regions(d);
>  }
>  
>  /* region mapping */
> @@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
>      /* Handle real device's MMIO/PIO BARs */
>      xen_pt_register_regions(s);
>  
> +    /* Setup VGA bios for passthroughed gfx */
> +    if (xen_pt_setup_vga(&s->real_device) < 0) {
> +        XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx failed!\n");
> +        xen_host_pci_device_put(&s->real_device);
> +        return -1;
> +    }
> +
>      /* reinitialize each config register to be emulated */
>      if (xen_pt_config_init(s)) {
>          XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 942dc60..4d3a18d 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -298,5 +298,9 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
>      return s->msix && s->msix->bar_index == bar;
>  }
>  
> +extern int xen_has_gfx_passthru;
> +int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
> +int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
> +int xen_pt_setup_vga(XenHostPCIDevice *dev);
>  
>  #endif /* !XEN_PT_H */
> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
> new file mode 100644
> index 0000000..e1f0724
> --- /dev/null
> +++ b/hw/xen/xen_pt_graphics.c
> @@ -0,0 +1,177 @@
> +/*
> + * graphics passthrough
> + */
> +#include "xen_pt.h"
> +#include "xen-host-pci-device.h"
> +#include "hw/xen/xen_backend.h"
> +
> +static int is_vga_passthrough(XenHostPCIDevice *dev)
> +{
> +    return (xen_has_gfx_passthru
> +            && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> +}
> +
> +/*
> + * register VGA resources for the domain with assigned gfx
> + */
> +int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
> +{
> +    int ret = 0;
> +
> +    if (!is_vga_passthrough(dev)) {
> +        return ret;
> +    }
> +
> +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
> +            0x3B0, 0xA, DPCI_ADD_MAPPING);
> +
> +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
> +            0x3C0, 0x20, DPCI_ADD_MAPPING);
> +
> +    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
> +            0xa0000 >> XC_PAGE_SHIFT,
> +            0xa0000 >> XC_PAGE_SHIFT,
> +            0x20,
> +            DPCI_ADD_MAPPING);
> +
> +    if (ret) {
> +        XEN_PT_ERR(NULL, "VGA region mapping failed\n");

It would be actually useful to know _which_ of them failed. Perhaps
you could structure this a bit differently and do:


struct _args {
        uint32_t gport;
        uint32_t mport;
        uint32_t nport;
};

        struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(args); i++) {
		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport, args[i]..)
		if (ret) {
			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x pages failed with rc:%d\n",
					... fill in the values)
			return ret;
	}	
		
	
> +    }
> +
> +    return ret;
> +}
> +
> +/*
> + * unregister VGA resources for the domain with assigned gfx
> + */
> +int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
> +{
> +    int ret = 0;
> +
> +    if (!is_vga_passthrough(dev)) {
> +        return ret;
> +    }
> +
> +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
> +            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
> +
> +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
> +            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
> +
> +    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
> +            0xa0000 >> XC_PAGE_SHIFT,
> +            0xa0000 >> XC_PAGE_SHIFT,
> +            20,
> +            DPCI_REMOVE_MAPPING);
> +
> +    if (ret) {
> +        XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
> +    }
> +

The same pattern as above.

> +    return ret;
> +}
> +
> +static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev)
> +{
> +    char rom_file[64];
> +    FILE *fp;
> +    uint8_t val;
> +    struct stat st;
> +    uint16_t magic = 0;
> +
> +    snprintf(rom_file, sizeof(rom_file),
> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",

I think the format changed to be: /%04x:%02x:%02x.%d in Linux
(see pci_setup_device in drivers/pci/probe.c) - not that it makes
that much difference as the function is only up to 7.

> +             dev->domain, dev->bus, dev->dev,
> +             dev->func);
> +
> +    if (stat(rom_file, &st)) {
> +        return -1;

ENODEV?

> +    }
> +
> +    if (access(rom_file, F_OK)) {
> +        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
> +                    rom_file);
> +        return -1;

EPERM?
> +    }
> +
> +    /* Write "1" to the ROM file to enable it */
> +    fp = fopen(rom_file, "r+");
> +    if (fp == NULL) {

EACCESS ?

> +        return -1;
> +    }
> +    val = 1;
> +    if (fwrite(&val, 1, 1, fp) != 1) {
> +        goto close_rom;
> +    }
> +    fseek(fp, 0, SEEK_SET);
> +
> +    /*
> +     * Check if it a real bios extension.
> +     * The magic number is 0xAA55.
> +     */
> +    if (fread(&magic, sizeof(magic), 1, fp)) {
> +        goto close_rom;
> +    }

Don't you want to do that before you write '1' in it?

> +
> +    if (magic != 0xAA55) {
> +        goto close_rom;
> +    }
> +    fseek(fp, 0, SEEK_SET);
> +
> +    if (!fread(buf, 1, st.st_size, fp)) {
> +        XEN_PT_ERR(NULL, "pci-assign: Cannot read from host %s", rom_file);
> +        XEN_PT_LOG(NULL, "Device option ROM contents are probably invalid "
> +                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
> +                     "or load from file with romfile=\n");
> +    }
> +
> +close_rom:
> +    /* Write "0" to disable ROM */
> +    fseek(fp, 0, SEEK_SET);
> +    val = 0;
> +    if (!fwrite(&val, 1, 1, fp)) {
> +        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");

Should we return -1? (after closing the file of course)

> +    }
> +    fclose(fp);
> +    return st.st_size;

Ah, that is why your return -1! In that case I presume the caller of this
function will check the 'errno' to find the underlaying issue
> +}
> +
> +/* Allocated 128K for the vga bios */
> +#define VGA_BIOS_DEFAULT_SIZE (0x20000)
> +
> +int xen_pt_setup_vga(XenHostPCIDevice *dev)
> +{
> +    unsigned char *bios = NULL;
> +    int bios_size = 0;
> +    char *c = NULL;
> +    char checksum = 0;
> +    int rc = 0;
> +
> +    if (!is_vga_passthrough(dev)) {
> +        return rc;
> +    }
> +
> +    bios = g_malloc0(VGA_BIOS_DEFAULT_SIZE);
> +
> +    bios_size = get_vgabios(bios, dev);
> +    if (bios_size == 0 || bios_size > VGA_BIOS_DEFAULT_SIZE) {
> +        XEN_PT_ERR(NULL, "vga bios size (0x%x) is invalid!\n", bios_size);

Um, with an error code, the 'bios size (0xfffffffff)' is going to show up.
Why don't you an extra code to check for this , like:

	if (bios_size < 0)
		XEN_PT_ERR(NULL,"Error %d (%s) getting BIOS!\n", errno, strerror(errno));
	else
		.. the other error.

> +        rc = -1;

> +        goto out;
> +    }
> +
> +    /* Adjust the bios checksum */
> +    for (c = (char *)bios; c < ((char *)bios + bios_size); c++) {
> +        checksum += *c;
> +    }
> +    if (checksum) {
> +        bios[bios_size - 1] -= checksum;
> +        XEN_PT_LOG(NULL, "vga bios checksum is adjusted!\n");
> +    }
> +
> +    /* Currently we fixed this address as a primary. */
> +    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
> +out:
> +    g_free(bios);
> +    return rc;
> +}
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 781af14..cece134 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1047,6 +1047,15 @@ STEXI
>  Rotate graphical output some deg left (only PXA LCD).
>  ETEXI
>  
> +DEF("gfx_passthru", 0, QEMU_OPTION_gfx_passthru,
> +    "-gfx_passthru   enable Intel IGD passthrough by XEN\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -gfx_passthru
> +@findex -gfx_passthru
> +Enable Intel IGD passthrough by XEN
> +ETEXI
> +
>  DEF("vga", HAS_ARG, QEMU_OPTION_vga,
>      "-vga [std|cirrus|vmware|qxl|xenfb|tcx|cg3|none]\n"
>      "                select video card type\n", QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index 73e0661..c86e95f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1436,6 +1436,13 @@ static void smp_parse(QemuOpts *opts)
>  
>  }
>  
> +/* We still need this for compatibility with XEN. */
> +int xen_has_gfx_passthru;
> +static void xen_gfx_passthru(const char *optarg)
> +{
> +    xen_has_gfx_passthru = 1;
> +}
> +
>  static void configure_realtime(QemuOpts *opts)
>  {
>      bool enable_mlock;
> @@ -2988,7 +2995,6 @@ int main(int argc, char **argv, char **envp)
>      const char *trace_file = NULL;
>      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                          1024 * 1024;
> -
>      atexit(qemu_run_exit_notifiers);
>      error_set_progname(argv[0]);
>      qemu_init_exec_dir(argv[0]);
> @@ -3956,6 +3962,9 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  configure_msg(opts);
>                  break;
> +            case QEMU_OPTION_gfx_passthru:
> +                xen_gfx_passthru(optarg);
> +                break;
>              default:
>                  os_parse_cmd_args(popt->index, optarg);
>              }
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Fri, 16 May 2014, Konrad Rzeszutek Wilk wrote:
> On Fri, May 16, 2014 at 06:53:39PM +0800, Tiejun Chen wrote:
> > basic gfx passthrough support:
> > - add a vga type for gfx passthrough
> > - retrieve VGA bios from sysfs, then load it to guest 0xC0000
> > - register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx
> > 
> > The original patch is from Weidong Han <weidong.han@intel.com>
> > 
> > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> > Cc: Weidong Han <weidong.han@intel.com>
> > ---
> > v2:
> > 
> > * retrieve VGA bios from sysfs properly.
> > * redefine some function name.
> > 
> >  hw/xen/Makefile.objs         |   2 +-
> >  hw/xen/xen-host-pci-device.c |   5 ++
> >  hw/xen/xen-host-pci-device.h |   1 +
> >  hw/xen/xen_pt.c              |  10 +++
> >  hw/xen/xen_pt.h              |   4 +
> >  hw/xen/xen_pt_graphics.c     | 177 +++++++++++++++++++++++++++++++++++++++++++
> >  qemu-options.hx              |   9 +++
> >  vl.c                         |  11 ++-
> >  8 files changed, 217 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/xen/xen_pt_graphics.c
> > 
> > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> > index a0ca0aa..77b7dae 100644
> > --- a/hw/xen/Makefile.objs
> > +++ b/hw/xen/Makefile.objs
> > @@ -2,4 +2,4 @@
> >  common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
> >  
> >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> > -obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
> > +obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o xen_pt_graphics.o
> > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> > index 743b37b..a54b7de 100644
> > --- a/hw/xen/xen-host-pci-device.c
> > +++ b/hw/xen/xen-host-pci-device.c
> > @@ -376,6 +376,11 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> >          goto error;
> >      }
> >      d->irq = v;
> > +    rc = xen_host_pci_get_hex_value(d, "class", &v);
> > +    if (rc) {
> > +        goto error;
> > +    }
> > +    d->class_code = v;
> >      d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
> >  
> >      return 0;
> > diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> > index c2486f0..f1e1c30 100644
> > --- a/hw/xen/xen-host-pci-device.h
> > +++ b/hw/xen/xen-host-pci-device.h
> > @@ -25,6 +25,7 @@ typedef struct XenHostPCIDevice {
> >  
> >      uint16_t vendor_id;
> >      uint16_t device_id;
> > +    uint32_t class_code;
> >      int irq;
> >  
> >      XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1];
> > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > index be4220b..a0113ea 100644
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -450,6 +450,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
> >                     d->rom.size, d->rom.base_addr);
> >      }
> >  
> > +    xen_pt_register_vga_regions(d);
> >      return 0;
> >  }
> >  
> > @@ -470,6 +471,8 @@ static void xen_pt_unregister_regions(XenPCIPassthroughState *s)
> >      if (d->rom.base_addr && d->rom.size) {
> >          memory_region_destroy(&s->rom);
> >      }
> > +
> > +    xen_pt_unregister_vga_regions(d);
> >  }
> >  
> >  /* region mapping */
> > @@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
> >      /* Handle real device's MMIO/PIO BARs */
> >      xen_pt_register_regions(s);
> >  
> > +    /* Setup VGA bios for passthroughed gfx */
> > +    if (xen_pt_setup_vga(&s->real_device) < 0) {
> > +        XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx failed!\n");
> > +        xen_host_pci_device_put(&s->real_device);
> > +        return -1;
> > +    }
> > +
> >      /* reinitialize each config register to be emulated */
> >      if (xen_pt_config_init(s)) {
> >          XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > index 942dc60..4d3a18d 100644
> > --- a/hw/xen/xen_pt.h
> > +++ b/hw/xen/xen_pt.h
> > @@ -298,5 +298,9 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
> >      return s->msix && s->msix->bar_index == bar;
> >  }
> >  
> > +extern int xen_has_gfx_passthru;
> > +int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
> > +int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
> > +int xen_pt_setup_vga(XenHostPCIDevice *dev);
> >  
> >  #endif /* !XEN_PT_H */
> > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
> > new file mode 100644
> > index 0000000..e1f0724
> > --- /dev/null
> > +++ b/hw/xen/xen_pt_graphics.c
> > @@ -0,0 +1,177 @@
> > +/*
> > + * graphics passthrough
> > + */
> > +#include "xen_pt.h"
> > +#include "xen-host-pci-device.h"
> > +#include "hw/xen/xen_backend.h"
> > +
> > +static int is_vga_passthrough(XenHostPCIDevice *dev)
> > +{
> > +    return (xen_has_gfx_passthru
> > +            && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
> > +}
> > +
> > +/*
> > + * register VGA resources for the domain with assigned gfx
> > + */
> > +int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
> > +{
> > +    int ret = 0;
> > +
> > +    if (!is_vga_passthrough(dev)) {
> > +        return ret;
> > +    }
> > +
> > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
> > +            0x3B0, 0xA, DPCI_ADD_MAPPING);

The original code does:

    ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0,
                        0x3B0, 0xC, DPCI_ADD_MAPPING);

why are we remapping fewer ports now?


> > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
> > +            0x3C0, 0x20, DPCI_ADD_MAPPING);
> > +
> > +    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
> > +            0xa0000 >> XC_PAGE_SHIFT,
> > +            0xa0000 >> XC_PAGE_SHIFT,
> > +            0x20,
> > +            DPCI_ADD_MAPPING);
> > +
> > +    if (ret) {
> > +        XEN_PT_ERR(NULL, "VGA region mapping failed\n");
> 
> It would be actually useful to know _which_ of them failed. Perhaps
> you could structure this a bit differently and do:
> 
> 
> struct _args {
>         uint32_t gport;
>         uint32_t mport;
>         uint32_t nport;
> };
> 
>         struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
> 	unsigned int i;
> 
> 	for (i = 0; i < ARRAY_SIZE(args); i++) {
> 		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport, args[i]..)
> 		if (ret) {
> 			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x pages failed with rc:%d\n",
> 					... fill in the values)
> 			return ret;
> 	}	
> 		
> 	
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +/*
> > + * unregister VGA resources for the domain with assigned gfx
> > + */
> > +int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
> > +{
> > +    int ret = 0;
> > +
> > +    if (!is_vga_passthrough(dev)) {
> > +        return ret;
> > +    }
> > +
> > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
> > +            0x3B0, 0xC, DPCI_REMOVE_MAPPING);

But here we are unmapping 0xC ioports....


> > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
> > +            0x3C0, 0x20, DPCI_REMOVE_MAPPING);
> > +
> > +    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
> > +            0xa0000 >> XC_PAGE_SHIFT,
> > +            0xa0000 >> XC_PAGE_SHIFT,
> > +            20,
> > +            DPCI_REMOVE_MAPPING);
> > +
> > +    if (ret) {
> > +        XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
> > +    }
> > +
> 
> The same pattern as above.
> 
> > +    return ret;
> > +}
> > +
> > +static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev)
> > +{
> > +    char rom_file[64];
> > +    FILE *fp;
> > +    uint8_t val;
> > +    struct stat st;
> > +    uint16_t magic = 0;
> > +
> > +    snprintf(rom_file, sizeof(rom_file),
> > +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",
> 
> I think the format changed to be: /%04x:%02x:%02x.%d in Linux
> (see pci_setup_device in drivers/pci/probe.c) - not that it makes
> that much difference as the function is only up to 7.
> 
> > +             dev->domain, dev->bus, dev->dev,
> > +             dev->func);
> > +
> > +    if (stat(rom_file, &st)) {
> > +        return -1;
> 
> ENODEV?
> 
> > +    }
> > +
> > +    if (access(rom_file, F_OK)) {
> > +        XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
> > +                    rom_file);
> > +        return -1;
> 
> EPERM?
> > +    }
> > +
> > +    /* Write "1" to the ROM file to enable it */
> > +    fp = fopen(rom_file, "r+");
> > +    if (fp == NULL) {
> 
> EACCESS ?
> 
> > +        return -1;
> > +    }
> > +    val = 1;
> > +    if (fwrite(&val, 1, 1, fp) != 1) {
> > +        goto close_rom;
> > +    }
> > +    fseek(fp, 0, SEEK_SET);
> > +
> > +    /*
> > +     * Check if it a real bios extension.
> > +     * The magic number is 0xAA55.
> > +     */
> > +    if (fread(&magic, sizeof(magic), 1, fp)) {
> > +        goto close_rom;
> > +    }
> 
> Don't you want to do that before you write '1' in it?
> 
> > +
> > +    if (magic != 0xAA55) {
> > +        goto close_rom;
> > +    }
> > +    fseek(fp, 0, SEEK_SET);
> > +
> > +    if (!fread(buf, 1, st.st_size, fp)) {
> > +        XEN_PT_ERR(NULL, "pci-assign: Cannot read from host %s", rom_file);
> > +        XEN_PT_LOG(NULL, "Device option ROM contents are probably invalid "
> > +                     "(check dmesg).\nSkip option ROM probe with rombar=0, "
> > +                     "or load from file with romfile=\n");
> > +    }
> > +
> > +close_rom:
> > +    /* Write "0" to disable ROM */
> > +    fseek(fp, 0, SEEK_SET);
> > +    val = 0;
> > +    if (!fwrite(&val, 1, 1, fp)) {
> > +        XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file");
> 
> Should we return -1? (after closing the file of course)
> 
> > +    }
> > +    fclose(fp);
> > +    return st.st_size;
> 
> Ah, that is why your return -1! In that case I presume the caller of this
> function will check the 'errno' to find the underlaying issue
> > +}
> > +
> > +/* Allocated 128K for the vga bios */
> > +#define VGA_BIOS_DEFAULT_SIZE (0x20000)

The old default was 0x10000, why are we changing it?


> > +int xen_pt_setup_vga(XenHostPCIDevice *dev)
> > +{
> > +    unsigned char *bios = NULL;
> > +    int bios_size = 0;
> > +    char *c = NULL;
> > +    char checksum = 0;
> > +    int rc = 0;
> > +
> > +    if (!is_vga_passthrough(dev)) {
> > +        return rc;
> > +    }
> > +
> > +    bios = g_malloc0(VGA_BIOS_DEFAULT_SIZE);
> > +
> > +    bios_size = get_vgabios(bios, dev);
> > +    if (bios_size == 0 || bios_size > VGA_BIOS_DEFAULT_SIZE) {
> > +        XEN_PT_ERR(NULL, "vga bios size (0x%x) is invalid!\n", bios_size);
> 
> Um, with an error code, the 'bios size (0xfffffffff)' is going to show up.
> Why don't you an extra code to check for this , like:
> 
> 	if (bios_size < 0)
> 		XEN_PT_ERR(NULL,"Error %d (%s) getting BIOS!\n", errno, strerror(errno));
> 	else
> 		.. the other error.
> 
> > +        rc = -1;
> 
> > +        goto out;
> > +    }
> > +
> > +    /* Adjust the bios checksum */
> > +    for (c = (char *)bios; c < ((char *)bios + bios_size); c++) {
> > +        checksum += *c;
> > +    }
> > +    if (checksum) {
> > +        bios[bios_size - 1] -= checksum;
> > +        XEN_PT_LOG(NULL, "vga bios checksum is adjusted!\n");
> > +    }
> > +
> > +    /* Currently we fixed this address as a primary. */
> > +    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
> > +out:
> > +    g_free(bios);
> > +    return rc;
> > +}
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 781af14..cece134 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1047,6 +1047,15 @@ STEXI
> >  Rotate graphical output some deg left (only PXA LCD).
> >  ETEXI
> >  
> > +DEF("gfx_passthru", 0, QEMU_OPTION_gfx_passthru,
> > +    "-gfx_passthru   enable Intel IGD passthrough by XEN\n",
> > +    QEMU_ARCH_ALL)
> > +STEXI
> > +@item -gfx_passthru
> > +@findex -gfx_passthru
> > +Enable Intel IGD passthrough by XEN
> > +ETEXI
> > +
> >  DEF("vga", HAS_ARG, QEMU_OPTION_vga,
> >      "-vga [std|cirrus|vmware|qxl|xenfb|tcx|cg3|none]\n"
> >      "                select video card type\n", QEMU_ARCH_ALL)
> > diff --git a/vl.c b/vl.c
> > index 73e0661..c86e95f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1436,6 +1436,13 @@ static void smp_parse(QemuOpts *opts)
> >  
> >  }
> >  
> > +/* We still need this for compatibility with XEN. */
> > +int xen_has_gfx_passthru;
> > +static void xen_gfx_passthru(const char *optarg)
> > +{
> > +    xen_has_gfx_passthru = 1;
> > +}
> > +
> >  static void configure_realtime(QemuOpts *opts)
> >  {
> >      bool enable_mlock;
> > @@ -2988,7 +2995,6 @@ int main(int argc, char **argv, char **envp)
> >      const char *trace_file = NULL;
> >      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> >                                          1024 * 1024;
> > -
> >      atexit(qemu_run_exit_notifiers);
> >      error_set_progname(argv[0]);
> >      qemu_init_exec_dir(argv[0]);
> > @@ -3956,6 +3962,9 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  configure_msg(opts);
> >                  break;
> > +            case QEMU_OPTION_gfx_passthru:
> > +                xen_gfx_passthru(optarg);
> > +                break;
> >              default:
> >                  os_parse_cmd_args(popt->index, optarg);
> >              }
> > -- 
> > 1.9.1
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Monday, May 19, 2014 8:10 PM
> To: Konrad Rzeszutek Wilk
> Cc: Chen, Tiejun; anthony.perard@citrix.com; stefano.stabellini@eu.citrix.com;
> mst@redhat.com; Kelly.Zytaruk@amd.com; peter.maydell@linaro.org;
> xen-devel@lists.xensource.com; weidong.han@intel.com; Kay, Allen M;
> qemu-devel@nongnu.org; jean.guyader@eu.citrix.com;
> anthony@codemonkey.ws; Zhang, Yang Z
> Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics
> passthrough support
> 
> On Fri, 16 May 2014, Konrad Rzeszutek Wilk wrote:
> > On Fri, May 16, 2014 at 06:53:39PM +0800, Tiejun Chen wrote:
> > > basic gfx passthrough support:
> > > - add a vga type for gfx passthrough
> > > - retrieve VGA bios from sysfs, then load it to guest 0xC0000
> > > - register/unregister legacy VGA I/O ports and MMIOs for
> > > passthroughed gfx
> > >
> > > The original patch is from Weidong Han <weidong.han@intel.com>
> > >
> > > Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> > > Cc: Weidong Han <weidong.han@intel.com>
> > > ---
> > > v2:
> > >
> > > * retrieve VGA bios from sysfs properly.
> > > * redefine some function name.
> > >
> > >  hw/xen/Makefile.objs         |   2 +-
> > >  hw/xen/xen-host-pci-device.c |   5 ++
> > >  hw/xen/xen-host-pci-device.h |   1 +
> > >  hw/xen/xen_pt.c              |  10 +++
> > >  hw/xen/xen_pt.h              |   4 +
> > >  hw/xen/xen_pt_graphics.c     | 177
> +++++++++++++++++++++++++++++++++++++++++++
> > >  qemu-options.hx              |   9 +++
> > >  vl.c                         |  11 ++-
> > >  8 files changed, 217 insertions(+), 2 deletions(-)  create mode
> > > 100644 hw/xen/xen_pt_graphics.c
> > >
> > > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs index
> > > a0ca0aa..77b7dae 100644
> > > --- a/hw/xen/Makefile.objs
> > > +++ b/hw/xen/Makefile.objs
> > > @@ -2,4 +2,4 @@
> > >  common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o
> xen_devconfig.o
> > >
> > >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
> > > -obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o
> > > xen_pt_msi.o
> > > +obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o
> > > +xen_pt_msi.o xen_pt_graphics.o
> > > diff --git a/hw/xen/xen-host-pci-device.c
> > > b/hw/xen/xen-host-pci-device.c index 743b37b..a54b7de 100644
> > > --- a/hw/xen/xen-host-pci-device.c
> > > +++ b/hw/xen/xen-host-pci-device.c
> > > @@ -376,6 +376,11 @@ int xen_host_pci_device_get(XenHostPCIDevice
> *d, uint16_t domain,
> > >          goto error;
> > >      }
> > >      d->irq = v;
> > > +    rc = xen_host_pci_get_hex_value(d, "class", &v);
> > > +    if (rc) {
> > > +        goto error;
> > > +    }
> > > +    d->class_code = v;
> > >      d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
> > >
> > >      return 0;
> > > diff --git a/hw/xen/xen-host-pci-device.h
> > > b/hw/xen/xen-host-pci-device.h index c2486f0..f1e1c30 100644
> > > --- a/hw/xen/xen-host-pci-device.h
> > > +++ b/hw/xen/xen-host-pci-device.h
> > > @@ -25,6 +25,7 @@ typedef struct XenHostPCIDevice {
> > >
> > >      uint16_t vendor_id;
> > >      uint16_t device_id;
> > > +    uint32_t class_code;
> > >      int irq;
> > >
> > >      XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1]; diff --git
> > > a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index be4220b..a0113ea 100644
> > > --- a/hw/xen/xen_pt.c
> > > +++ b/hw/xen/xen_pt.c
> > > @@ -450,6 +450,7 @@ static int
> xen_pt_register_regions(XenPCIPassthroughState *s)
> > >                     d->rom.size, d->rom.base_addr);
> > >      }
> > >
> > > +    xen_pt_register_vga_regions(d);
> > >      return 0;
> > >  }
> > >
> > > @@ -470,6 +471,8 @@ static void
> xen_pt_unregister_regions(XenPCIPassthroughState *s)
> > >      if (d->rom.base_addr && d->rom.size) {
> > >          memory_region_destroy(&s->rom);
> > >      }
> > > +
> > > +    xen_pt_unregister_vga_regions(d);
> > >  }
> > >
> > >  /* region mapping */
> > > @@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
> > >      /* Handle real device's MMIO/PIO BARs */
> > >      xen_pt_register_regions(s);
> > >
> > > +    /* Setup VGA bios for passthroughed gfx */
> > > +    if (xen_pt_setup_vga(&s->real_device) < 0) {
> > > +        XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx
> failed!\n");
> > > +        xen_host_pci_device_put(&s->real_device);
> > > +        return -1;
> > > +    }
> > > +
> > >      /* reinitialize each config register to be emulated */
> > >      if (xen_pt_config_init(s)) {
> > >          XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
> > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h index
> > > 942dc60..4d3a18d 100644
> > > --- a/hw/xen/xen_pt.h
> > > +++ b/hw/xen/xen_pt.h
> > > @@ -298,5 +298,9 @@ static inline bool
> xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
> > >      return s->msix && s->msix->bar_index == bar;  }
> > >
> > > +extern int xen_has_gfx_passthru;
> > > +int xen_pt_register_vga_regions(XenHostPCIDevice *dev); int
> > > +xen_pt_unregister_vga_regions(XenHostPCIDevice *dev); int
> > > +xen_pt_setup_vga(XenHostPCIDevice *dev);
> > >
> > >  #endif /* !XEN_PT_H */
> > > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c new
> > > file mode 100644 index 0000000..e1f0724
> > > --- /dev/null
> > > +++ b/hw/xen/xen_pt_graphics.c
> > > @@ -0,0 +1,177 @@
> > > +/*
> > > + * graphics passthrough
> > > + */
> > > +#include "xen_pt.h"
> > > +#include "xen-host-pci-device.h"
> > > +#include "hw/xen/xen_backend.h"
> > > +
> > > +static int is_vga_passthrough(XenHostPCIDevice *dev) {
> > > +    return (xen_has_gfx_passthru
> > > +            && ((dev->class_code >> 0x8) ==
> > > +PCI_CLASS_DISPLAY_VGA)); }
> > > +
> > > +/*
> > > + * register VGA resources for the domain with assigned gfx  */ int
> > > +xen_pt_register_vga_regions(XenHostPCIDevice *dev) {
> > > +    int ret = 0;
> > > +
> > > +    if (!is_vga_passthrough(dev)) {
> > > +        return ret;
> > > +    }
> > > +
> > > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
> > > +            0x3B0, 0xA, DPCI_ADD_MAPPING);
> 
> The original code does:
> 
>     ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0,
>                         0x3B0, 0xC, DPCI_ADD_MAPPING);
> 
> why are we remapping fewer ports now?
> 
> 
> > > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
> > > +            0x3C0, 0x20, DPCI_ADD_MAPPING);
> > > +
> > > +    ret |= xc_domain_memory_mapping(xen_xc, xen_domid,
> > > +            0xa0000 >> XC_PAGE_SHIFT,
> > > +            0xa0000 >> XC_PAGE_SHIFT,
> > > +            0x20,
> > > +            DPCI_ADD_MAPPING);
> > > +
> > > +    if (ret) {
> > > +        XEN_PT_ERR(NULL, "VGA region mapping failed\n");
> >
> > It would be actually useful to know _which_ of them failed. Perhaps
> > you could structure this a bit differently and do:
> >
> >
> > struct _args {
> >         uint32_t gport;
> >         uint32_t mport;
> >         uint32_t nport;
> > };
> >
> >         struct _args args[3] = {{ 0x3B0, 0x3B0, 0xA }, {...}};
> > 	unsigned int i;
> >
> > 	for (i = 0; i < ARRAY_SIZE(args); i++) {
> > 		ret = xc_domain_ioport_mapping(xen_xc, xen_domid, args[i].gport,
> args[i]..)
> > 		if (ret) {
> > 			XEN_PT_ERR_(NULL, "VGA region mapping of 0x%lx for 0x%x
> pages failed with rc:%d\n",
> > 					... fill in the values)
> > 			return ret;
> > 	}
> >
> >
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +/*
> > > + * unregister VGA resources for the domain with assigned gfx  */
> > > +int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) {
> > > +    int ret = 0;
> > > +
> > > +    if (!is_vga_passthrough(dev)) {
> > > +        return ret;
> > > +    }
> > > +
> > > +    ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
> > > +            0x3B0, 0xC, DPCI_REMOVE_MAPPING);
> 
> But here we are unmapping 0xC ioports....
> 

They're typos and they should be introduced by the original v1.

Actually I also notice this yesterday when I try to improve this part with your comments, you can see I already unify these places when I reply to you online.

Thanks
Tiejun