Message ID | 1409567807-18633-2-git-send-email-tiejun.chen@intel.com |
---|---|
State | New |
Headers | show |
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
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
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.
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
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 --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 */
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