diff mbox

[v3,1/1] hw/pci-assign: split pci-assign.c

Message ID 1409567807-18633-2-git-send-email-tiejun.chen@intel.com
State New
Headers show

Commit Message

Tiejun Chen Sept. 1, 2014, 10:36 a.m. UTC 
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 <tiejun.chen@intel.com>
---
 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

Comments

Michael S. Tsirkin Sept. 1, 2014, 10:46 a.m. UTC | #1 
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 <tiejun.chen@intel.com>
> ---
>  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);
>  }
> diff --git a/include/hw/pci/pci-assign.h b/include/hw/pci/pci-assign.h
> new file mode 100644
> index 0000000..6d6bf93
> --- /dev/null
> +++ b/include/hw/pci/pci-assign.h
> @@ -0,0 +1,16 @@
> +/*
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Just split from hw/i386/kvm/pci-assign.c.
> + */
> +#ifndef PCI_ASSIGN_H
> +#define PCI_ASSIGN_H
> +
> +#include "hw/pci/pci.h"
> +
> +int pci_assign_dev_load_option_rom(PCIDevice *dev, struct Object *owner,
> +                                   void *ptr, unsigned int domain,
> +                                   unsigned int bus, unsigned int slot,
> +                                   unsigned int function);
> +#endif /* PCI_ASSIGN_H */
> -- 
> 1.9.1
Tiejun Chen Sept. 1, 2014, 10:56 a.m. UTC | #2 
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 <tiejun.chen@intel.com>
>> ---
>>   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 :)

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;
+
+    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;
+    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
Michael S. Tsirkin Sept. 1, 2014, 11:08 a.m. UTC | #3 
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 <tiejun.chen@intel.com>
> >>---
> >>  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.
Tiejun Chen Sept. 1, 2014, 11:15 a.m. UTC | #4 
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 <tiejun.chen@intel.com>
>>>> ---
>>>>   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
Michael S. Tsirkin Sept. 1, 2014, 11:19 a.m. UTC | #5 
On Mon, Sep 01, 2014 at 07:15:42PM +0800, Chen, Tiejun wrote:
> 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

I'll know when I see it :)
diff mbox

Patch

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,
+                                   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;
+
+    pci_assign_dev_load_option_rom(&dev->dev, OBJECT(dev), ptr,
+                                   dev->host.domain, dev->host.bus,
+                                   dev->host.slot, dev->host.function);
 }
diff --git a/include/hw/pci/pci-assign.h b/include/hw/pci/pci-assign.h
new file mode 100644
index 0000000..6d6bf93
--- /dev/null
+++ b/include/hw/pci/pci-assign.h
@@ -0,0 +1,16 @@ 
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Just split from hw/i386/kvm/pci-assign.c.
+ */
+#ifndef PCI_ASSIGN_H
+#define PCI_ASSIGN_H
+
+#include "hw/pci/pci.h"
+
+int pci_assign_dev_load_option_rom(PCIDevice *dev, struct Object *owner,
+                                   void *ptr, unsigned int domain,
+                                   unsigned int bus, unsigned int slot,
+                                   unsigned int function);
+#endif /* PCI_ASSIGN_H */