qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c


From: Chen, Tiejun
Subject: Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
Date: Mon, 01 Sep 2014 19:15:42 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

On 2014/9/1 19:08, Michael S. Tsirkin wrote:
On Mon, Sep 01, 2014 at 06:56:55PM +0800, Chen, Tiejun wrote:
On 2014/9/1 18:46, Michael S. Tsirkin wrote:
On Mon, Sep 01, 2014 at 06:36:47PM +0800, Tiejun Chen wrote:
We will try to reuse assign_dev_load_option_rom in xen side, and
especially its a good beginning to unify pci assign codes both on
kvm and xen in the future.

Signed-off-by: Tiejun Chen <address@hidden>
---
  hw/i386/kvm/pci-assign.c    | 46 +++++++++++++++++++++++++++++----------------
  include/hw/pci/pci-assign.h | 16 ++++++++++++++++
  2 files changed, 46 insertions(+), 16 deletions(-)
  create mode 100644 include/hw/pci/pci-assign.h

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 17c7d6dc..fdc7b64 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -37,6 +37,7 @@
  #include "hw/pci/pci.h"
  #include "hw/pci/msi.h"
  #include "kvm_i386.h"
+#include "hw/pci/pci-assign.h"

  #define MSIX_PAGE_SIZE 0x1000

@@ -1896,37 +1897,39 @@ type_init(assign_register_types)
   * load the corresponding ROM data to RAM. If an error occurs while loading an
   * option ROM, we just ignore that option ROM and continue with the next one.
   */
-static void assigned_dev_load_option_rom(AssignedDevice *dev)
+int pci_assign_dev_load_option_rom(PCIDevice *dev, struct Object *owner,
+                                   void *ptr, unsigned int domain,

ptr parameter seems unused.

+                                   unsigned int bus, unsigned int slot,
+                                   unsigned int function)
  {
      char name[32], rom_file[64];
      FILE *fp;
      uint8_t val;
      struct stat st;
-    void *ptr;
+    int size = 0;

      /* If loading ROM from file, pci handles it */
-    if (dev->dev.romfile || !dev->dev.rom_bar) {
-        return;
+    if (dev->romfile || !dev->rom_bar) {
+        return -1;
      }

      snprintf(rom_file, sizeof(rom_file),
               "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",
-             dev->host.domain, dev->host.bus, dev->host.slot,
-             dev->host.function);
+             domain, bus, slot, function);

      if (stat(rom_file, &st)) {
-        return;
+        return -1;
      }

      if (access(rom_file, F_OK)) {
          error_report("pci-assign: Insufficient privileges for %s", rom_file);
-        return;
+        return -1;
      }

      /* Write "1" to the ROM file to enable it */
      fp = fopen(rom_file, "r+");
      if (fp == NULL) {
-        return;
+        return -1;
      }
      val = 1;
      if (fwrite(&val, 1, 1, fp) != 1) {
@@ -1934,11 +1937,10 @@ static void assigned_dev_load_option_rom(AssignedDevice 
*dev)
      }
      fseek(fp, 0, SEEK_SET);

-    snprintf(name, sizeof(name), "%s.rom",
-            object_get_typename(OBJECT(dev)));
-    memory_region_init_ram(&dev->dev.rom, OBJECT(dev), name, st.st_size);
-    vmstate_register_ram(&dev->dev.rom, &dev->dev.qdev);
-    ptr = memory_region_get_ram_ptr(&dev->dev.rom);
+    snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner));
+    memory_region_init_ram(&dev->rom, owner, name, st.st_size);
+    vmstate_register_ram(&dev->rom, &dev->qdev);
+    ptr = memory_region_get_ram_ptr(&dev->rom);
      memset(ptr, 0xff, st.st_size);

      if (!fread(ptr, 1, st.st_size, fp)) {
@@ -1949,8 +1951,9 @@ static void assigned_dev_load_option_rom(AssignedDevice 
*dev)
          goto close_rom;
      }

-    pci_register_bar(&dev->dev, PCI_ROM_SLOT, 0, &dev->dev.rom);
-    dev->dev.has_rom = true;
+    pci_register_bar(dev, PCI_ROM_SLOT, 0, &dev->rom);
+    dev->has_rom = true;
+    size = st.st_size;
  close_rom:
      /* Write "0" to disable ROM */
      fseek(fp, 0, SEEK_SET);
@@ -1959,4 +1962,15 @@ close_rom:
          DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
      }
      fclose(fp);
+
+    return size;
+}
+
+static void assigned_dev_load_option_rom(AssignedDevice *dev)
+{
+    void *ptr = NULL;
+

This is never modified, I don't think you need this
variable.

+    pci_assign_dev_load_option_rom(&dev->dev, OBJECT(dev), ptr,
+                                   dev->host.domain, dev->host.bus,
+                                   dev->host.slot, dev->host.function);
  }

In patch 0/1, there's a little bit background for this usage in xen side :)

I still don't get it. the value you pass is never used.


Currently we only support that as primary display device, so we need to copy
vBIOS from BAR to 0xc0000. Here I picked some codes fragments up:

+static int get_vgabios(XenPCIPassthroughState *s, void *ptr,
+                       XenHostPCIDevice *dev)
+{
+    int size = 0;
+


this is wrong too, you don't need = 0 here.

+    size = pci_assign_dev_load_option_rom(&s->dev, OBJECT(dev), ptr,
+                                          dev->domain, dev->bus,
+                                          dev->dev, dev->func);
+
+    return size;
+}
+
+int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
+{
+    void *bios = NULL;
+    int bios_size = 0;

and here

+    int rc = 0;
+
+    if (!is_vga_passthrough(dev)) {
+        return rc;
+    }
+
+    bios_size = get_vgabios(s, bios, dev);
+    if (!bios || !bios_size) {
+        XEN_PT_ERR(NULL, "VGA: getting VBIOS!\n");
+        rc = -1;
+        goto out;
+    }
+
+    /* Currently we fixed this address as a primary. */
+    cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
+out:
+    g_free(bios);
+    return rc;
+}

Of course, if you have a better idea, please tell me.

Thanks
Tiejun

don't initialize variables just to override them a couple
of lines down.


Michael,

Thanks for this preview of IGD stuff patches, I already fixed these two points in my tree.

Then this mean the v3 patch should be fine to you?

Thanks
Tiejun



reply via email to

[Prev in Thread] Current Thread [Next in Thread]