|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support
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@xxxxxxxxx>
>
> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> Cc: Weidong Han <weidong.han@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |