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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> Sent: Friday, May 16, 2014 10:06 PM
> To: Chen, Tiejun
> Cc: anthony.perard@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> mst@xxxxxxxxxx; Kelly.Zytaruk@xxxxxxx; peter.maydell@xxxxxxxxxx;
> xen-devel@xxxxxxxxxxxxxxxxxxx; weidong.han@xxxxxxxxx; Kay, Allen M;
> qemu-devel@xxxxxxxxxx; jean.guyader@xxxxxxxxxxxxx;
> anthony@xxxxxxxxxxxxx; 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel