[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic
From: |
Chen, Tiejun |
Subject: |
Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support |
Date: |
Mon, 19 May 2014 09:42:23 +0000 |
> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:address@hidden
> Sent: Friday, May 16, 2014 10:06 PM
> To: Chen, Tiejun
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; Kay, Allen M;
> address@hidden; address@hidden;
> address@hidden; Zhang, Yang Z
> Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics
> passthrough support
>
[snip]
> > +/*
> > + * 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;
> }
>
Looks good so what about this based on the original,
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -14,34 +14,73 @@ static int is_vga_passthrough(XenHostPCIDevice *dev)
&& ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
}
+typedef struct VGARegion {
+ int type; /* Memory or port I/O */
+ uint64_t guest_base_addr;
+ uint64_t machine_base_addr;
+ uint64_t size; /* size of the region */
+ int rc;
+} VGARegion;
+
+#define IORESOURCE_IO 0x00000100
+#define IORESOURCE_MEM 0x00000200
+
+static struct VGARegion vga_args[] = {
+ {
+ .type = IORESOURCE_IO,
+ .guest_base_addr = 0x3B0,
+ .machine_base_addr = 0x3B0,
+ .size = 0xC,
+ .rc = -1,
+ },
+ {
+ .type = IORESOURCE_IO,
+ .guest_base_addr = 0x3C0,
+ .machine_base_addr = 0x3C0,
+ .size = 0x20,
+ .rc = -1,
+ },
+ {
+ .type = IORESOURCE_MEM,
+ .guest_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+ .machine_base_addr = 0xa0000 >> XC_PAGE_SHIFT,
+ .size = 0x20,
+ .rc = -1,
+ },
+};
+
/*
* register VGA resources for the domain with assigned gfx
*/
int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
{
- int ret = 0;
+ int i = 0;
if (!is_vga_passthrough(dev)) {
- return ret;
+ return -1;
}
- 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);
+ for(i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+ if (vga_args[i].type == IORESOURCE_IO) {
+ vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+ vga_args[i].guest_base_addr,
+ vga_args[i].machine_base_addr,
+ vga_args[i].size, DPCI_ADD_MAPPING);
+ } else {
+ vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+ vga_args[i].guest_base_addr,
+ vga_args[i].machine_base_addr,
+ vga_args[i].size, DPCI_ADD_MAPPING);
+ }
- if (ret) {
- XEN_PT_ERR(NULL, "VGA region mapping failed\n");
+ if (vga_args[i].rc) {
+ XEN_PT_ERR(NULL, "VGA %s mapping failed! (rc: %i)\n",
+ vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+ vga_args[i].rc);
+ }
}
- return ret;
+ return 0;
}
/*
@@ -49,29 +88,33 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev)
*/
int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
{
- int ret = 0;
+ int i = 0;
if (!is_vga_passthrough(dev)) {
- return ret;
+ return -1;
}
- 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);
+ for(i = 0 ; i < ARRAY_SIZE(vga_args); i++) {
+ if (vga_args[i].type == IORESOURCE_IO) {
+ vga_args[i].rc = xc_domain_ioport_mapping(xen_xc, xen_domid,
+ vga_args[i].guest_base_addr,
+ vga_args[i].machine_base_addr,
+ vga_args[i].size, DPCI_REMOVE_MAPPING);
+ } else {
+ vga_args[i].rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+ vga_args[i].guest_base_addr,
+ vga_args[i].machine_base_addr,
+ vga_args[i].size, DPCI_REMOVE_MAPPING);
+ }
- if (ret) {
- XEN_PT_ERR(NULL, "VGA region unmapping failed\n");
+ if (vga_args[i].rc) {
+ XEN_PT_ERR(NULL, "VGA %s unmapping failed! (rc: %i)\n",
+ vga_args[i].type == IORESOURCE_IO ? "ioport" : "memory",
+ vga_args[i].rc);
+ }
}
- return ret;
+ return 0;
}
static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev)
>
> > + }
> > +
> > + 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.
>
See the 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.
Will improved this as you pointed.
>
> > + dev->domain, dev->bus, dev->dev,
> > + dev->func);
> > +
> > + if (stat(rom_file, &st)) {
> > + return -1;
>
> ENODEV?
Fixed.
>
> > + }
> > +
> > + if (access(rom_file, F_OK)) {
> > + XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s",
> > + rom_file);
> > + return -1;
>
> EPERM?
Fixed.
> > + }
> > +
> > + /* Write "1" to the ROM file to enable it */
> > + fp = fopen(rom_file, "r+");
> > + if (fp == NULL) {
>
> EACCESS ?
>
Fixed.
> > + 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?
>
Definitely, but I really did this above this line :)
> > +
> > + 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)
>
Fixed.
> > + }
> > + 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
I will modify something here and xen_pt_setup_vga(). Please see next version.
Thanks
Tiejun
- [Qemu-devel] [v2][PATCH 0/8] xen: add Intel IGD passthrough support, Tiejun Chen, 2014/05/16
- [Qemu-devel] [v2][PATCH 1/8] pci: use bitmap to manage registe/runregister pci device, Tiejun Chen, 2014/05/16
- [Qemu-devel] [v2][PATCH 2/8] pci: provide a way to reserve some specific devfn, Tiejun Chen, 2014/05/16
- [Qemu-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support, Tiejun Chen, 2014/05/16
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support, Konrad Rzeszutek Wilk, 2014/05/16
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support,
Chen, Tiejun <=
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support, Konrad Rzeszutek Wilk, 2014/05/19
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support, Chen, Tiejun, 2014/05/20
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support, Stefano Stabellini, 2014/05/19
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support, Chen, Tiejun, 2014/05/20
[Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD, Tiejun Chen, 2014/05/16