From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43084) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmK69-0003az-MZ for qemu-devel@nongnu.org; Mon, 19 May 2014 05:43:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WmK65-0003Mz-QT for qemu-devel@nongnu.org; Mon, 19 May 2014 05:43:09 -0400 Received: from mga09.intel.com ([134.134.136.24]:60773) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmK65-0003Mh-BD for qemu-devel@nongnu.org; Mon, 19 May 2014 05:43:05 -0400 From: "Chen, Tiejun" Date: Mon, 19 May 2014 09:42:23 +0000 Message-ID: References: <1400237624-8505-1-git-send-email-tiejun.chen@intel.com> <1400237624-8505-4-git-send-email-tiejun.chen@intel.com> <20140516140615.GA3154@phenom.dumpdata.com> In-Reply-To: <20140516140615.GA3154@phenom.dumpdata.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konrad Rzeszutek Wilk Cc: "peter.maydell@linaro.org" , "xen-devel@lists.xensource.com" , "mst@redhat.com" , "stefano.stabellini@eu.citrix.com" , "Kay, Allen M" , "Kelly.Zytaruk@amd.com" , "qemu-devel@nongnu.org" , "Zhang, Yang Z" , "anthony@codemonkey.ws" , "anthony.perard@citrix.com" > -----Original Message----- > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] > Sent: Friday, May 16, 2014 10:06 PM > To: Chen, Tiejun > Cc: 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 grap= hics > passthrough support >=20 [snip] > > +/* > > + * register VGA resources for the domain with assigned gfx */ int > > +xen_pt_register_vga_regions(XenHostPCIDevice *dev) { > > + int ret =3D 0; > > + > > + if (!is_vga_passthrough(dev)) { > > + return ret; > > + } > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, > > + 0x3B0, 0xA, DPCI_ADD_MAPPING); > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, > > + 0x3C0, 0x20, DPCI_ADD_MAPPING); > > + > > + ret |=3D 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"); >=20 > It would be actually useful to know _which_ of them failed. Perhaps you c= ould > structure this a bit differently and do: >=20 >=20 > struct _args { > uint32_t gport; > uint32_t mport; > uint32_t nport; > }; >=20 > struct _args args[3] =3D {{ 0x3B0, 0x3B0, 0xA }, {...}}; > unsigned int i; >=20 > for (i =3D 0; i < ARRAY_SIZE(args); i++) { > ret =3D 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; > } >=20 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) =3D=3D PCI_CLASS_DISPLAY_VGA)); } =20 +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[] =3D { + { + .type =3D IORESOURCE_IO, + .guest_base_addr =3D 0x3B0, + .machine_base_addr =3D 0x3B0, + .size =3D 0xC, + .rc =3D -1, + }, + { + .type =3D IORESOURCE_IO, + .guest_base_addr =3D 0x3C0, + .machine_base_addr =3D 0x3C0, + .size =3D 0x20, + .rc =3D -1, + }, + { + .type =3D IORESOURCE_MEM, + .guest_base_addr =3D 0xa0000 >> XC_PAGE_SHIFT, + .machine_base_addr =3D 0xa0000 >> XC_PAGE_SHIFT, + .size =3D 0x20, + .rc =3D -1, + }, +}; + /* * register VGA resources for the domain with assigned gfx */ int xen_pt_register_vga_regions(XenHostPCIDevice *dev) { - int ret =3D 0; + int i =3D 0; =20 if (!is_vga_passthrough(dev)) { - return ret; + return -1; } =20 - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, - 0x3B0, 0xA, DPCI_ADD_MAPPING); - - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, - 0x3C0, 0x20, DPCI_ADD_MAPPING); - - ret |=3D xc_domain_memory_mapping(xen_xc, xen_domid, - 0xa0000 >> XC_PAGE_SHIFT, - 0xa0000 >> XC_PAGE_SHIFT, - 0x20, - DPCI_ADD_MAPPING); + for(i =3D 0 ; i < ARRAY_SIZE(vga_args); i++) { + if (vga_args[i].type =3D=3D IORESOURCE_IO) { + vga_args[i].rc =3D 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 =3D 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); + } =20 - 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 =3D=3D IORESOURCE_IO ? "ioport" : "me= mory", + vga_args[i].rc); + } } =20 - return ret; + return 0; } =20 /* @@ -49,29 +88,33 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev) */ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) { - int ret =3D 0; + int i =3D 0; =20 if (!is_vga_passthrough(dev)) { - return ret; + return -1; } =20 - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, - 0x3B0, 0xC, DPCI_REMOVE_MAPPING); - - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, - 0x3C0, 0x20, DPCI_REMOVE_MAPPING); - - ret |=3D xc_domain_memory_mapping(xen_xc, xen_domid, - 0xa0000 >> XC_PAGE_SHIFT, - 0xa0000 >> XC_PAGE_SHIFT, - 20, - DPCI_REMOVE_MAPPING); + for(i =3D 0 ; i < ARRAY_SIZE(vga_args); i++) { + if (vga_args[i].type =3D=3D IORESOURCE_IO) { + vga_args[i].rc =3D 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 =3D 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); + } =20 - 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 =3D=3D IORESOURCE_IO ? "ioport" : "me= mory", + vga_args[i].rc); + } } =20 - return ret; + return 0; } =20 static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev) >=20 > > + } > > + > > + return ret; > > +} > > + > > +/* > > + * unregister VGA resources for the domain with assigned gfx */ int > > +xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) { > > + int ret =3D 0; > > + > > + if (!is_vga_passthrough(dev)) { > > + return ret; > > + } > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, > > + 0x3B0, 0xC, DPCI_REMOVE_MAPPING); > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, > > + 0x3C0, 0x20, DPCI_REMOVE_MAPPING); > > + > > + ret |=3D 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"); > > + } > > + >=20 > The same pattern as above. >=20 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 =3D 0; > > + > > + snprintf(rom_file, sizeof(rom_file), > > + "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom", >=20 > 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. >=20 > > + dev->domain, dev->bus, dev->dev, > > + dev->func); > > + > > + if (stat(rom_file, &st)) { > > + return -1; >=20 > ENODEV? Fixed. >=20 > > + } > > + > > + if (access(rom_file, F_OK)) { > > + XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s", > > + rom_file); > > + return -1; >=20 > EPERM? Fixed. > > + } > > + > > + /* Write "1" to the ROM file to enable it */ > > + fp =3D fopen(rom_file, "r+"); > > + if (fp =3D=3D NULL) { >=20 > EACCESS ? >=20 Fixed. > > + return -1; > > + } > > + val =3D 1; > > + if (fwrite(&val, 1, 1, fp) !=3D 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; > > + } >=20 > Don't you want to do that before you write '1' in it? >=20 Definitely, but I really did this above this line :) > > + > > + if (magic !=3D 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=3D0, " > > + "or load from file with romfile=3D\n"); > > + } > > + > > +close_rom: > > + /* Write "0" to disable ROM */ > > + fseek(fp, 0, SEEK_SET); > > + val =3D 0; > > + if (!fwrite(&val, 1, 1, fp)) { > > + XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file"); >=20 > Should we return -1? (after closing the file of course) >=20 Fixed. > > + } > > + fclose(fp); > > + return st.st_size; >=20 > 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 versio= n. Thanks Tiejun From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support Date: Mon, 19 May 2014 09:42:23 +0000 Message-ID: References: <1400237624-8505-1-git-send-email-tiejun.chen@intel.com> <1400237624-8505-4-git-send-email-tiejun.chen@intel.com> <20140516140615.GA3154@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20140516140615.GA3154@phenom.dumpdata.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Konrad Rzeszutek Wilk Cc: "peter.maydell@linaro.org" , "xen-devel@lists.xensource.com" , "mst@redhat.com" , "stefano.stabellini@eu.citrix.com" , "Kay, Allen M" , "Kelly.Zytaruk@amd.com" , "qemu-devel@nongnu.org" , "Zhang, Yang Z" , "anthony@codemonkey.ws" , "anthony.perard@citrix.com" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] > Sent: Friday, May 16, 2014 10:06 PM > To: Chen, Tiejun > Cc: 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 grap= hics > passthrough support >=20 [snip] > > +/* > > + * register VGA resources for the domain with assigned gfx */ int > > +xen_pt_register_vga_regions(XenHostPCIDevice *dev) { > > + int ret =3D 0; > > + > > + if (!is_vga_passthrough(dev)) { > > + return ret; > > + } > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, > > + 0x3B0, 0xA, DPCI_ADD_MAPPING); > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, > > + 0x3C0, 0x20, DPCI_ADD_MAPPING); > > + > > + ret |=3D 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"); >=20 > It would be actually useful to know _which_ of them failed. Perhaps you c= ould > structure this a bit differently and do: >=20 >=20 > struct _args { > uint32_t gport; > uint32_t mport; > uint32_t nport; > }; >=20 > struct _args args[3] =3D {{ 0x3B0, 0x3B0, 0xA }, {...}}; > unsigned int i; >=20 > for (i =3D 0; i < ARRAY_SIZE(args); i++) { > ret =3D 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; > } >=20 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) =3D=3D PCI_CLASS_DISPLAY_VGA)); } =20 +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[] =3D { + { + .type =3D IORESOURCE_IO, + .guest_base_addr =3D 0x3B0, + .machine_base_addr =3D 0x3B0, + .size =3D 0xC, + .rc =3D -1, + }, + { + .type =3D IORESOURCE_IO, + .guest_base_addr =3D 0x3C0, + .machine_base_addr =3D 0x3C0, + .size =3D 0x20, + .rc =3D -1, + }, + { + .type =3D IORESOURCE_MEM, + .guest_base_addr =3D 0xa0000 >> XC_PAGE_SHIFT, + .machine_base_addr =3D 0xa0000 >> XC_PAGE_SHIFT, + .size =3D 0x20, + .rc =3D -1, + }, +}; + /* * register VGA resources for the domain with assigned gfx */ int xen_pt_register_vga_regions(XenHostPCIDevice *dev) { - int ret =3D 0; + int i =3D 0; =20 if (!is_vga_passthrough(dev)) { - return ret; + return -1; } =20 - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, - 0x3B0, 0xA, DPCI_ADD_MAPPING); - - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, - 0x3C0, 0x20, DPCI_ADD_MAPPING); - - ret |=3D xc_domain_memory_mapping(xen_xc, xen_domid, - 0xa0000 >> XC_PAGE_SHIFT, - 0xa0000 >> XC_PAGE_SHIFT, - 0x20, - DPCI_ADD_MAPPING); + for(i =3D 0 ; i < ARRAY_SIZE(vga_args); i++) { + if (vga_args[i].type =3D=3D IORESOURCE_IO) { + vga_args[i].rc =3D 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 =3D 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); + } =20 - 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 =3D=3D IORESOURCE_IO ? "ioport" : "me= mory", + vga_args[i].rc); + } } =20 - return ret; + return 0; } =20 /* @@ -49,29 +88,33 @@ int xen_pt_register_vga_regions(XenHostPCIDevice *dev) */ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) { - int ret =3D 0; + int i =3D 0; =20 if (!is_vga_passthrough(dev)) { - return ret; + return -1; } =20 - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, - 0x3B0, 0xC, DPCI_REMOVE_MAPPING); - - ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, - 0x3C0, 0x20, DPCI_REMOVE_MAPPING); - - ret |=3D xc_domain_memory_mapping(xen_xc, xen_domid, - 0xa0000 >> XC_PAGE_SHIFT, - 0xa0000 >> XC_PAGE_SHIFT, - 20, - DPCI_REMOVE_MAPPING); + for(i =3D 0 ; i < ARRAY_SIZE(vga_args); i++) { + if (vga_args[i].type =3D=3D IORESOURCE_IO) { + vga_args[i].rc =3D 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 =3D 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); + } =20 - 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 =3D=3D IORESOURCE_IO ? "ioport" : "me= mory", + vga_args[i].rc); + } } =20 - return ret; + return 0; } =20 static int get_vgabios(unsigned char *buf, XenHostPCIDevice *dev) >=20 > > + } > > + > > + return ret; > > +} > > + > > +/* > > + * unregister VGA resources for the domain with assigned gfx */ int > > +xen_pt_unregister_vga_regions(XenHostPCIDevice *dev) { > > + int ret =3D 0; > > + > > + if (!is_vga_passthrough(dev)) { > > + return ret; > > + } > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0, > > + 0x3B0, 0xC, DPCI_REMOVE_MAPPING); > > + > > + ret |=3D xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0, > > + 0x3C0, 0x20, DPCI_REMOVE_MAPPING); > > + > > + ret |=3D 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"); > > + } > > + >=20 > The same pattern as above. >=20 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 =3D 0; > > + > > + snprintf(rom_file, sizeof(rom_file), > > + "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom", >=20 > 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. >=20 > > + dev->domain, dev->bus, dev->dev, > > + dev->func); > > + > > + if (stat(rom_file, &st)) { > > + return -1; >=20 > ENODEV? Fixed. >=20 > > + } > > + > > + if (access(rom_file, F_OK)) { > > + XEN_PT_ERR(NULL, "pci-assign: Insufficient privileges for %s", > > + rom_file); > > + return -1; >=20 > EPERM? Fixed. > > + } > > + > > + /* Write "1" to the ROM file to enable it */ > > + fp =3D fopen(rom_file, "r+"); > > + if (fp =3D=3D NULL) { >=20 > EACCESS ? >=20 Fixed. > > + return -1; > > + } > > + val =3D 1; > > + if (fwrite(&val, 1, 1, fp) !=3D 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; > > + } >=20 > Don't you want to do that before you write '1' in it? >=20 Definitely, but I really did this above this line :) > > + > > + if (magic !=3D 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=3D0, " > > + "or load from file with romfile=3D\n"); > > + } > > + > > +close_rom: > > + /* Write "0" to disable ROM */ > > + fseek(fp, 0, SEEK_SET); > > + val =3D 0; > > + if (!fwrite(&val, 1, 1, fp)) { > > + XEN_PT_LOG("%s\n", "Failed to disable pci-sysfs rom file"); >=20 > Should we return -1? (after closing the file of course) >=20 Fixed. > > + } > > + fclose(fp); > > + return st.st_size; >=20 > 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 versio= n. Thanks Tiejun