Re: [Qemu-devel] [Xen-devel] [v2][PATCH 3/8] xen, gfx passthrough: basic graphics passthrough support

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, May 19, 2014 at 09:42:23AM +0000, Chen, Tiejun wrote:
> > -----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 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
>