* [Qemu-devel] [v3][PATCH 0/1] qemu:pci-assign: try to pci-assign.c
@ 2014-09-01 10:36 Tiejun Chen
2014-09-01 10:36 ` [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c Tiejun Chen
0 siblings, 1 reply; 7+ messages in thread
From: Tiejun Chen @ 2014-09-01 10:36 UTC (permalink / raw)
To: mst, aliguori; +Cc: qemu-devel
v3:
* Don't move out those common structures.
* Just include minimum required head file.
* Rename dev_load_option_rom with pci_assign_dev_load_option_rom to avoid
global namespace pollution.
v2:
* v1 is making so much code inline, so try to move it to an out of line file.
* rename pci-assign not pci_assign.
A you know I'm working on supporting IGD passthrough.
Here we need load VGABIOS to work out IGD case. Obviously something may
be duplicated to kvm codes, we should unify some codes but looks its not
easy to finish that in short time. So as Michael suggestion, at least
we'd better split assigned_dev_load_option_rom to reuse on both kvm
and xen.
I don't finish all IGD stuff patches but here I'd like to post
some related codes to show how to use assigned_dev_load_option_rom()
lately.
+static int get_vgabios(XenPCIPassthroughState *s, void *ptr,
+ XenHostPCIDevice *dev)
+{
+ int size = 0;
+
+ size = 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;
+ }
...
----------------------------------------------------------------
Tiejun Chen (1):
hw/pci-assign: split pci-assign.c
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
Thanks
Tiejun
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
2014-09-01 10:36 [Qemu-devel] [v3][PATCH 0/1] qemu:pci-assign: try to pci-assign.c Tiejun Chen
@ 2014-09-01 10:36 ` Tiejun Chen
2014-09-01 10:46 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Tiejun Chen @ 2014-09-01 10:36 UTC (permalink / raw)
To: mst, aliguori; +Cc: qemu-devel
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,
+ 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 */
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
2014-09-01 10:36 ` [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c Tiejun Chen
@ 2014-09-01 10:46 ` Michael S. Tsirkin
2014-09-01 10:56 ` Chen, Tiejun
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-09-01 10:46 UTC (permalink / raw)
To: Tiejun Chen; +Cc: qemu-devel, aliguori
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
2014-09-01 10:46 ` Michael S. Tsirkin
@ 2014-09-01 10:56 ` Chen, Tiejun
2014-09-01 11:08 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Chen, Tiejun @ 2014-09-01 10:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, aliguori
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
2014-09-01 10:56 ` Chen, Tiejun
@ 2014-09-01 11:08 ` Michael S. Tsirkin
2014-09-01 11:15 ` Chen, Tiejun
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-09-01 11:08 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: qemu-devel, aliguori
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
2014-09-01 11:08 ` Michael S. Tsirkin
@ 2014-09-01 11:15 ` Chen, Tiejun
2014-09-01 11:19 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Chen, Tiejun @ 2014-09-01 11:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, aliguori
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c
2014-09-01 11:15 ` Chen, Tiejun
@ 2014-09-01 11:19 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-09-01 11:19 UTC (permalink / raw)
To: Chen, Tiejun; +Cc: qemu-devel, aliguori
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 :)
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-01 11:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-01 10:36 [Qemu-devel] [v3][PATCH 0/1] qemu:pci-assign: try to pci-assign.c Tiejun Chen
2014-09-01 10:36 ` [Qemu-devel] [v3][PATCH 1/1] hw/pci-assign: split pci-assign.c Tiejun Chen
2014-09-01 10:46 ` Michael S. Tsirkin
2014-09-01 10:56 ` Chen, Tiejun
2014-09-01 11:08 ` Michael S. Tsirkin
2014-09-01 11:15 ` Chen, Tiejun
2014-09-01 11:19 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.