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

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

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