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

[Chen, Tiejun]

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 <address@hidden>
> > ---
> >   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