From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOPat-0008LJ-Od for qemu-devel@nongnu.org; Mon, 01 Sep 2014 07:16:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOPao-0006lI-Mn for qemu-devel@nongnu.org; Mon, 01 Sep 2014 07:16:19 -0400 Received: from mga11.intel.com ([192.55.52.93]:47692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOPao-0006l7-BC for qemu-devel@nongnu.org; Mon, 01 Sep 2014 07:16:14 -0400 Message-ID: <5404555E.4000904@intel.com> Date: Mon, 01 Sep 2014 19:15:42 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1409567807-18633-1-git-send-email-tiejun.chen@intel.com> <1409567807-18633-2-git-send-email-tiejun.chen@intel.com> <20140901104629.GA22493@redhat.com> <540450F7.7060302@intel.com> <20140901110850.GB23979@redhat.com> In-Reply-To: <20140901110850.GB23979@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, aliguori@amazon.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 >>>> --- >>>> 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