[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: |
Konrad Rzeszutek Wilk |
Subject: |
Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support |
Date: |
Mon, 19 May 2014 09:35:52 -0400 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Mon, May 19, 2014 at 09:42:23AM +0000, Chen, Tiejun wrote:
> > -----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;
I think you still need to return a non-zero value in case of failure.
> }
>
> 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;
I think you still need to return a non-zero value in case of failure.
> > > +}
> > > +
> > > +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, 2014/05/19
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support,
Konrad Rzeszutek Wilk <=
- 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
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD, Konrad Rzeszutek Wilk, 2014/05/16
- Re: [Qemu-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD, Gerd Hoffmann, 2014/05/19
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD, Fabio Fantoni, 2014/05/19
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD, Zhang, Yang Z, 2014/05/19
- Re: [Qemu-devel] [Xen-devel] [v2][PATCH 4/8] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD, Chen, Tiejun, 2014/05/19