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

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

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