Kay, Allen M
2010-Jun-04 00:18 UTC
[Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
Added Calpella and Sandybridge integrated graphics pass-through support, consolidated graphics pass-through code into pt-graphics.c. Signed-off-by: Allen Kay <allen.m.kay@intel.com<mailto:allen.m.kay@intel.com>> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-04 15:29 UTC
Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Fri, 4 Jun 2010, Kay, Allen M wrote:> > Added Calpella and Sandybridge integrated graphics pass-through support, consolidated graphics pass-through code into > pt-graphics.c. > > Signed-off-by: Allen Kay <allen.m.kay@intel.com> >some comments follow> diff --git a/hw/pass-through.c b/hw/pass-through.c > index 5a76e8d..971c7f1 100644 > --- a/hw/pass-through.c > +++ b/hw/pass-through.c > @@ -1865,50 +1865,6 @@ static int pt_dev_is_virtfn(struct pci_dev *dev) > return rc; > } > > -/* > - * register VGA resources for the domain with assigned gfx > - */ > -static int register_vga_regions(struct pt_dev *real_device) > -{ > - int ret = 0; > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0, > - 0x3B0, 0xC, DPCI_ADD_MAPPING); > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0, > - 0x3C0, 0x20, DPCI_ADD_MAPPING); > - > - ret |= xc_domain_memory_mapping(xc_handle, domid, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0x20, > - DPCI_ADD_MAPPING); > - > - return ret; > -} > - > -/* > - * unregister VGA resources for the domain with assigned gfx > - */ > -static int unregister_vga_regions(struct pt_dev *real_device) > -{ > - int ret = 0; > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0, > - 0x3B0, 0xC, DPCI_REMOVE_MAPPING); > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0, > - 0x3C0, 0x20, DPCI_REMOVE_MAPPING); > - > - ret |= xc_domain_memory_mapping(xc_handle, domid, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0x20, > - DPCI_REMOVE_MAPPING); > - > - return ret; > -} > - > static int pt_register_regions(struct pt_dev *assigned_device) > { > int i = 0; > @@ -1970,17 +1926,7 @@ static int pt_register_regions(struct pt_dev *assigned_device) > PT_LOG("Expansion ROM registered (size=0x%08x base_addr=0x%08x)\n", > (uint32_t)(pci_dev->rom_size), (uint32_t)(pci_dev->rom_base_addr)); > } > - > - if ( gfx_passthru && (pci_dev->device_class == 0x0300) ) > - { > - ret = register_vga_regions(assigned_device); > - if ( ret != 0 ) > - { > - PT_LOG("VGA region mapping failed\n"); > - return ret; > - } > - } > - > + register_vga_regions(assigned_device); > return 0; > } > > @@ -2029,13 +1975,7 @@ static void pt_unregister_regions(struct pt_dev *assigned_device) > } > > } > - > - if ( gfx_passthru && (assigned_device->pci_dev->device_class == 0x0300) ) > - { > - ret = unregister_vga_regions(assigned_device); > - if ( ret != 0 ) > - PT_LOG("VGA region unmapping failed\n"); > - } > + unregister_vga_regions(assigned_device); > } > > static uint8_t find_cap_offset(struct pci_dev *pci_dev, uint8_t cap) > @@ -2097,46 +2037,66 @@ static uint32_t find_ext_cap_offset(struct pci_dev *pci_dev, uint32_t cap) > return 0; > } > > -u8 pt_pci_host_read_byte(int bus, int dev, int fn, u32 addr) > +static void pci_access_init(void) > { > - struct pci_dev *pci_dev; > - u8 val; > + struct pci_access *pci_access; > > - pci_dev = pci_get_dev(dpci_infos.pci_access, 0, bus, dev, fn); > - if ( !pci_dev ) > - return 0; > + if (dpci_infos.pci_access) > + return; > > - val = pci_read_byte(pci_dev, addr); > - pci_free_dev(pci_dev); > - return val; > + /* Initialize libpci */ > + pci_access = pci_alloc(); > + if ( pci_access == NULL ) { > + PT_LOG("Error: pci_access is NULL\n"); > + return -1; > + } > + pci_init(pci_access); > + pci_scan_bus(pci_access); > + dpci_infos.pci_access = pci_access; > } > > -u16 pt_pci_host_read_word(int bus, int dev, int fn, u32 addr) > +u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len) > { > + > struct pci_dev *pci_dev; > - u16 val; > + u32 val = -1; > > + pci_access_init(); > pci_dev = pci_get_dev(dpci_infos.pci_access, 0, bus, dev, fn); > if ( !pci_dev ) > return 0; > > - val = pci_read_word(pci_dev, addr); > - pci_free_dev(pci_dev); > + switch (len) > + { > + case 1: val = pci_read_byte(pci_dev, addr); break; > + case 2: val = pci_read_word(pci_dev, addr); break; > + case 4: val = pci_read_long(pci_dev, addr); break; > + default: > + fprintf(stderr, "error: pt_pci_host_read: invalid len = %d\n", len); > + } > return val; > } > > -u32 pt_pci_host_read_long(int bus, int dev, int fn, u32 addr) > +int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len) > { > struct pci_dev *pci_dev; > - u32 val; > + int ret = 0; > > + pci_access_init(); > pci_dev = pci_get_dev(dpci_infos.pci_access, 0, bus, dev, fn); > if ( !pci_dev ) > return 0; > > - val = pci_read_long(pci_dev, addr); > + switch (len) > + { > + case 1: ret = pci_write_byte(pci_dev, addr, val); break; > + case 2: ret = pci_write_word(pci_dev, addr, val); break; > + case 4: ret = pci_write_long(pci_dev, addr, val); break; > + default: > + fprintf(stderr, "error: pt_pci_host_write: invalid len = %d\n", len); > + } > pci_free_dev(pci_dev); > - return val; > + return ret; > } > > /* parse BAR */ > @@ -4200,92 +4160,6 @@ static int pt_pmcsr_reg_restore(struct pt_dev *ptdev, > return 0; > } > > -static int get_vgabios(unsigned char *buf) > -{ > - int fd; > - uint32_t bios_size = 0; > - uint32_t start = 0xC0000; > - uint16_t magic = 0; > - > - if ( (fd = open("/dev/mem", O_RDONLY)) < 0 ) > - { > - PT_LOG("Error: Can''t open /dev/mem: %s\n", strerror(errno)); > - return 0; > - } > - > - /* > - * Check if it a real bios extension. > - * The magic number is 0xAA55. > - */ > - if ( start != lseek(fd, start, SEEK_SET) ) > - goto out; > - if ( read(fd, &magic, 2) != 2 ) > - goto out; > - if ( magic != 0xAA55 ) > - goto out; > - > - /* Find the size of the rom extension */ > - if ( start != lseek(fd, start, SEEK_SET) ) > - goto out; > - if ( lseek(fd, 2, SEEK_CUR) != (start + 2) ) > - goto out; > - if ( read(fd, &bios_size, 1) != 1 ) > - goto out; > - > - /* This size is in 512 bytes */ > - bios_size *= 512; > - > - /* > - * Set the file to the begining of the rombios, > - * to start the copy. > - */ > - if ( start != lseek(fd, start, SEEK_SET) ) > - goto out; > - > - if ( bios_size != read(fd, buf, bios_size)) > - bios_size = 0; > - > -out: > - close(fd); > - return bios_size; > -} > - > -static int setup_vga_pt(void) > -{ > - unsigned char *bios = NULL; > - int bios_size = 0; > - char *c = NULL; > - char checksum = 0; > - int rc = 0; > - > - /* Allocated 64K for the vga bios */ > - if ( !(bios = malloc(64 * 1024)) ) > - return -1; > - > - bios_size = get_vgabios(bios); > - if ( bios_size == 0 || bios_size > 64 * 1024) > - { > - PT_LOG("vga bios size (0x%x) is invalid!\n", bios_size); > - rc = -1; > - goto out; > - } > - > - /* Adjust the bios checksum */ > - for ( c = (char*)bios; c < ((char*)bios + bios_size); c++ ) > - checksum += *c; > - if ( checksum ) > - { > - bios[bios_size - 1] -= checksum; > - PT_LOG("vga bios checksum is adjusted!\n"); > - } > - > - cpu_physical_memory_rw(0xc0000, bios, bios_size, 1); > - > -out: > - free(bios); > - return rc; > -} > - > static struct pt_dev * register_real_device(PCIBus *e_bus, > const char *e_dev_name, int e_devfn, uint8_t r_bus, uint8_t r_dev, > uint8_t r_func, uint32_t machine_irq, struct pci_access *pci_access, > @@ -4387,16 +4261,13 @@ static struct pt_dev * register_real_device(PCIBus *e_bus, > pt_register_regions(assigned_device); > > /* Setup VGA bios for passthroughed gfx */ > - if ( gfx_passthru && (assigned_device->pci_dev->device_class == 0x0300) ) > + if ( setup_vga_pt(assigned_device) < 0 ) > { > - rc = setup_vga_pt(); > - if ( rc < 0 ) > - { > - PT_LOG("Setup VGA BIOS of passthroughed gfx failed!\n"); > - return NULL; > - } > + PT_LOG("Setup VGA BIOS of passthroughed gfx failed!\n"); > + return NULL; > } > > + > /* reinitialize each config register to be emulated */ > rc = pt_config_init(assigned_device); > if ( rc < 0 ) { > @@ -4548,19 +4419,8 @@ int power_on_php_devfn(int devfn) > { > struct php_dev *php_dev = &dpci_infos.php_devs[devfn]; > struct pt_dev *pt_dev; > - struct pci_access *pci_access; > > - if (!dpci_infos.pci_access) { > - /* Initialize libpci */ > - pci_access = pci_alloc(); > - if ( pci_access == NULL ) { > - PT_LOG("Error: pci_access is NULL\n"); > - return -1; > - } > - pci_init(pci_access); > - pci_scan_bus(pci_access); > - dpci_infos.pci_access = pci_access; > - } > + pci_access_init(); > > pt_dev > register_real_device(dpci_infos.e_bus, > diff --git a/hw/pass-through.h b/hw/pass-through.h > index f8a0c73..6c5e8ca 100644 > --- a/hw/pass-through.h > +++ b/hw/pass-through.h > @@ -406,10 +406,16 @@ static inline pciaddr_t pt_pci_base_addr(pciaddr_t base) > } > > uint8_t pci_intx(struct pt_dev *ptdev); > - > -u8 pt_pci_host_read_byte(int bus, int dev, int fn, u32 addr); > -u16 pt_pci_host_read_word(int bus, int dev, int fn, u32 addr); > -u32 pt_pci_host_read_long(int bus, int dev, int fn, u32 addr); > +u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len); > +int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len); > +PCIBus *intel_pch_init(PCIBus *bus); > +int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); > +int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len); > +int register_vga_regions(struct pt_dev *real_device); > +int unregister_vga_regions(struct pt_dev *real_device); > +int setup_vga_pt(struct pt_dev *real_device); > +PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, > + uint16_t did, const char *name, uint16_t revision); > > #endif /* __PASSTHROUGH_H__ */ > > diff --git a/hw/pc.c b/hw/pc.c > index 9375951..7364cb8 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -42,6 +42,10 @@ > #include "virtio-console.h" > #include "hpet_emul.h" > > +#ifdef CONFIG_PASSTHROUGH > +#include "pass-through.h" > +#endif > + > /* output Bochs bios info messages */ > //#define DEBUG_BIOS > > @@ -978,6 +982,8 @@ vga_bios_error: > pci_bus = NULL; > } > > + intel_pch_init(pci_bus); > + > /* init basic PC hardware */ > register_ioport_write(0x80, 1, 1, ioport80_write, NULL); >You are missing a ifdef CONFIG_PASSTHROUGH here> diff --git a/hw/pci.c b/hw/pci.c > index b07e5ea..fa32ed9 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -39,24 +39,6 @@ extern int igd_passthru; > > //#define DEBUG_PCI > > -struct PCIBus { > - int bus_num; > - int devfn_min; > - pci_set_irq_fn set_irq; > - pci_map_irq_fn map_irq; > - uint32_t config_reg; /* XXX: suppress */ > - /* low level pic */ > - SetIRQFunc *low_set_irq; > - qemu_irq *irq_opaque; > - PCIDevice *devices[256]; > - PCIDevice *parent_dev; > - PCIBus *next; > - /* The bus IRQ state is the logical OR of the connected devices. > - Keep a count of the number of devices with raised IRQs. */ > - int nirq; > - int irq_count[]; > -}; > - > static void pci_update_mappings(PCIDevice *d); > static void pci_set_irq(void *opaque, int irq_num, int level); > > @@ -96,7 +78,7 @@ PCIBus *pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > return bus; > } > > -static PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq) > +PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq) > { > PCIBus *bus; > bus = qemu_mallocz(sizeof(PCIBus)); > @@ -590,6 +572,10 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > printf("pci_config_write: %s: addr=%02x val=%08x len=%d\n", > pci_dev->name, config_addr, val, len); > #endif > + > +#ifdef CONFIG_PASSTHROUGH > + if (igd_pci_write(pci_dev, config_addr, val, len) == 0) > +#endif > pci_dev->config_write(pci_dev, config_addr, val, len); > } > > @@ -598,7 +584,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > PCIBus *s = opaque; > PCIDevice *pci_dev; > int config_addr, bus_num; > - uint32_t val; > + uint32_t val = 0; > > bus_num = (addr >> 16) & 0xff; > while (s && s->bus_num != bus_num) > @@ -625,26 +611,8 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > config_addr = addr & 0xff; > > #ifdef CONFIG_PASSTHROUGH > - /* host bridge reads for IGD passthrough */ > - if ( igd_passthru && pci_dev->devfn == 0x00 ) { > - val = pci_dev->config_read(pci_dev, config_addr, len); > - > - if ( config_addr == 0x00 && len == 4 ) > - val = pt_pci_host_read_long(0, 0, 0, 0x00); > - else if ( config_addr == 0x02 ) // Device ID > - val = pt_pci_host_read_word(0, 0, 0, 0x02); > - else if ( config_addr == 0x52 ) // GMCH Graphics Control Register > - val = pt_pci_host_read_word(0, 0, 0, 0x52); > - else if ( config_addr == 0xa0 ) // GMCH Top of Memory Register > - val = pt_pci_host_read_word(0, 0, 0, 0xa0); > - goto done_config_read; > - } else if ( igd_passthru && pci_dev->devfn == 0x10 && > - config_addr == 0xfc ) { // read on IGD device > - val = 0; // use SMI to communicate with the system BIOS > - goto done_config_read; > - } > + if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0) > #endif > - > val = pci_dev->config_read(pci_dev, config_addr, len); > > done_config_read: > @@ -892,12 +860,7 @@ void pci_unplug_netifs(void) > } > } > > -typedef struct { > - PCIDevice dev; > - PCIBus *bus; > -} PCIBridge; > - > -static void pci_bridge_write_config(PCIDevice *d, > +void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { > PCIBridge *s = (PCIBridge *)d; > diff --git a/hw/pci.h b/hw/pci.h > index de5a4e1..18e7b6f 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -207,6 +207,32 @@ struct PCIDevice { > int irq_state[4]; > }; > > +typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); > +typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > + > +struct PCIBus { > + int bus_num; > + int devfn_min; > + pci_set_irq_fn set_irq; > + pci_map_irq_fn map_irq; > + uint32_t config_reg; /* XXX: suppress */ > + /* low level pic */ > + SetIRQFunc *low_set_irq; > + qemu_irq *irq_opaque; > + PCIDevice *devices[256]; > + PCIDevice *parent_dev; > + PCIBus *next; > + /* The bus IRQ state is the logical OR of the connected devices. > + Keep a count of the number of devices with raised IRQs. */ > + int nirq; > + int irq_count[]; > +}; > + > +typedef struct { > + PCIDevice dev; > + PCIBus *bus; > +} PCIBridge; > + > extern char direct_pci_str[]; > extern int direct_pci_msitranslate; > extern int direct_pci_power_mgmt; > @@ -235,8 +261,6 @@ void pci_default_write_config(PCIDevice *d, > void pci_device_save(PCIDevice *s, QEMUFile *f); > int pci_device_load(PCIDevice *s, QEMUFile *f); > > -typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); > -typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > PCIBus *pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > qemu_irq *pic, int devfn_min, int nirq); > > @@ -341,5 +365,9 @@ PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > /* pass-through.c */ > int pt_init(PCIBus *e_bus); > +void pci_bridge_write_config(PCIDevice *d, > + uint32_t address, uint32_t val, int len); > +PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq); > + > > #endif > diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c > new file mode 100644 > index 0000000..3b9ce95 > --- /dev/null > +++ b/hw/pt-graphics.c > @@ -0,0 +1,266 @@ > +/* > + * graphics passthrough > + */ > + > +#include "pass-through.h" > +#include "pci/header.h" > +#include "pci/pci.h" > +#include "pt-msi.h" > +#include "qemu-xen.h" > +#include "iomulti.h" > + > +#include <unistd.h> > +#include <sys/ioctl.h> > + > +extern int gfx_passthru; > +extern int igd_passthru; > + > +static int pch_irq_function(PCIDevice *pci_dev, int irq_num) > +{ > + PT_LOG("pch_irq_function called\n"); > + return irq_num; > +} > + > +PCIBus *intel_pch_init(PCIBus *bus) > +{ > + PCIBridge *pch; > + u16 vendor_id, device_id; > + u8 rev_id; > + > + if ( !gfx_passthru ) > + return NULL; > + > + vendor_id = pt_pci_host_read(0, 0x1f, 0, 0, 2); > + device_id = pt_pci_host_read(0, 0x1f, 0, 2, 2); > + rev_id = pt_pci_host_read(0, 0x1f, 0, 8, 1); > + > + pch = (PCIBridge *) > + pci_register_device(bus, "intel_bridge_1f", sizeof(PCIBridge), > + PCI_DEVFN(0x1f, 0), NULL, pci_bridge_write_config); > + > + pci_config_set_vendor_id(pch->dev.config, vendor_id); > + pci_config_set_device_id(pch->dev.config, device_id); > + > + pch->dev.config[0x04] = 0x06; // command = bus master, pci mem > + pch->dev.config[0x05] = 0x00; > + pch->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error > + pch->dev.config[0x07] = 0x00; // status = fast devsel > + pch->dev.config[0x08] = rev_id; > + pch->dev.config[0x09] = 0x00; // programming i/f > + pci_config_set_class(pch->dev.config, PCI_CLASS_BRIDGE_ISA); > + pch->dev.config[0x0D] = 0x10; // latency_timer > + pch->dev.config[0x0E] = 0x81; // header_type > + pch->dev.config[0x1E] = 0xa0; // secondary status > + > + pch->bus = pci_register_secondary_bus(&pch->dev, pch_irq_function); > + return pch->bus; > +} > +What happens if the hardware is a pre-Calpella Intel graphic card? Is it still going to work?> +int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) > +{ > + if ( !igd_passthru || (pci_dev->devfn != 0x00 ) ) > + return 0; > + > + switch (config_addr) > + { > + case 0x58: // PAVPC Offset > + pt_pci_host_write(0, 0, 0, config_addr, val, len); > + PT_LOG("pci_config_write: %x:%x.%x: addr=%x len=%x val=%x\n", > + pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn), config_addr, len, val); > + break; > + default: > + pci_dev->config_write(pci_dev, config_addr, val, len); > + } > + return 1; > +} > + > +int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) > +{ > + if ( !igd_passthru || (pci_dev->devfn != 0) ) > + return 0; > + > + switch (config_addr) > + { > + case 0x00: /* vendor id */ > + case 0x02: /* device id */ > + case 0x52: /* processor graphics control register */ > + case 0xa0: /* top of memory */ > + case 0xb0: /* ILK: BSM: should read from dev 2 offset 0x5c */ > + case 0x58: /* SNB: PAVPC Offset */ > + case 0xa4: /* SNB: graphics base of stolen memory */ > + case 0xa8: /* SNB: base of GTT stolen memory */ > + *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), > + 0, config_addr, len); > + PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n", > + pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn), config_addr, len, *val); > + > + break; > + default: > + *val = pci_dev->config_read(pci_dev, config_addr, len); > + } > + return 1; > +} > + > +/* > + * register VGA resources for the domain with assigned gfx > + */ > +int register_vga_regions(struct pt_dev *real_device) > +{ > + u32 igd_opregion, igd_bsm; > + int ret = 0; > + > + if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 ) > + return ret; > + > + ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0, > + 0x3B0, 0xC, DPCI_ADD_MAPPING); > + > + ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0, > + 0x3C0, 0x20, DPCI_ADD_MAPPING); > + > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + 0xa0000 >> XC_PAGE_SHIFT, > + 0xa0000 >> XC_PAGE_SHIFT, > + 0x20, > + DPCI_ADD_MAPPING); > + > + /* 1:1 map ASL Storage register value */ > + igd_opregion = pt_pci_host_read(0, 2, 0, 0xfc, 4); > + PT_LOG("register_vga: igd_opregion = %x\n", igd_opregion); > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + igd_opregion >> XC_PAGE_SHIFT, > + igd_opregion >> XC_PAGE_SHIFT, > + 2, > + DPCI_ADD_MAPPING);Again, what happens if the hardware is older? Do all Intel graphic cards have an opregion?> + igd_opregion = pt_pci_host_read(0, 2, 0, 0xfc, 4); > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + igd_opregion >> XC_PAGE_SHIFT, > + igd_opregion >> XC_PAGE_SHIFT, > + 2, > + DPCI_REMOVE_MAPPING); > +ditto _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Isaku Yamahata
2010-Jun-07 07:45 UTC
Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Thu, Jun 03, 2010 at 05:18:33PM -0700, Kay, Allen M wrote:> Added Calpella and Sandybridge integrated graphics pass-through support, > consolidated graphics pass-through code into pt-graphics.c. > > > > Signed-off-by: Allen Kay <allen.m.kay@intel.com> >Some minor comments. pci_{read, write}_block() would be better than switch(len) case 1: case 2: case4:... Is it really necessary to move PCIBus and PCIBridge to the header file? Doesn''t pci_bridge_init() work? Overriding pci config read/write methods of i440fx would be much cleaner than hooking pci_data_read/write. (pass igd_pci_read/write to pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL) -- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2010-Jun-08 21:15 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
Isaku, Thanks for the feedback.> pci_{read, write}_block() would be better than > switch(len) case 1: case 2: case4:...Done!> Is it really necessary to move PCIBus and PCIBridge to the header file? > Doesn''t pci_bridge_init() work?I changed to code to utilize pci_bridge_init(). However, I still need to move PCIBus and PCIBridge defines to the header file. The alternative is to pollute pc.c with graphics passthrough specific code.> Overriding pci config read/write methods of i440fx would be much cleaner > than hooking pci_data_read/write. (pass igd_pci_read/write to > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL)Doing this resulted in a lot of duplicated code and also force code path to change even when IGD passthrough is not used. To do it correctly, I also need to put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep the original patch. Allen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2010-Jun-08 21:46 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
Stephano, Thanks for your feedback. I add "#ifdef CONFIG_PASSTHROUGH" in the next version of the patch. I have tested the patch on a Intel Montevina software development platform and found it has the same behavior as current xen qemu upstream. Can others who have been playing with IGD passthrough give the patch a try on older platforms? Allen -----Original Message----- From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] Sent: Friday, June 04, 2010 8:29 AM To: Kay, Allen M Cc: xen-devel@lists.xensource.com; Ian Pratt; Han, Weidong; Ross Philipson; Jean Guyader Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge On Fri, 4 Jun 2010, Kay, Allen M wrote:> > Added Calpella and Sandybridge integrated graphics pass-through support, consolidated graphics pass-through code into > pt-graphics.c. > > Signed-off-by: Allen Kay <allen.m.kay@intel.com> >some comments follow> diff --git a/hw/pass-through.c b/hw/pass-through.c > index 5a76e8d..971c7f1 100644 > --- a/hw/pass-through.c > +++ b/hw/pass-through.c > @@ -1865,50 +1865,6 @@ static int pt_dev_is_virtfn(struct pci_dev *dev) > return rc; > } > > -/* > - * register VGA resources for the domain with assigned gfx > - */ > -static int register_vga_regions(struct pt_dev *real_device) > -{ > - int ret = 0; > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0, > - 0x3B0, 0xC, DPCI_ADD_MAPPING); > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0, > - 0x3C0, 0x20, DPCI_ADD_MAPPING); > - > - ret |= xc_domain_memory_mapping(xc_handle, domid, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0x20, > - DPCI_ADD_MAPPING); > - > - return ret; > -} > - > -/* > - * unregister VGA resources for the domain with assigned gfx > - */ > -static int unregister_vga_regions(struct pt_dev *real_device) > -{ > - int ret = 0; > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0, > - 0x3B0, 0xC, DPCI_REMOVE_MAPPING); > - > - ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0, > - 0x3C0, 0x20, DPCI_REMOVE_MAPPING); > - > - ret |= xc_domain_memory_mapping(xc_handle, domid, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0xa0000 >> XC_PAGE_SHIFT, > - 0x20, > - DPCI_REMOVE_MAPPING); > - > - return ret; > -} > - > static int pt_register_regions(struct pt_dev *assigned_device) > { > int i = 0; > @@ -1970,17 +1926,7 @@ static int pt_register_regions(struct pt_dev *assigned_device) > PT_LOG("Expansion ROM registered (size=0x%08x base_addr=0x%08x)\n", > (uint32_t)(pci_dev->rom_size), (uint32_t)(pci_dev->rom_base_addr)); > } > - > - if ( gfx_passthru && (pci_dev->device_class == 0x0300) ) > - { > - ret = register_vga_regions(assigned_device); > - if ( ret != 0 ) > - { > - PT_LOG("VGA region mapping failed\n"); > - return ret; > - } > - } > - > + register_vga_regions(assigned_device); > return 0; > } > > @@ -2029,13 +1975,7 @@ static void pt_unregister_regions(struct pt_dev *assigned_device) > } > > } > - > - if ( gfx_passthru && (assigned_device->pci_dev->device_class == 0x0300) ) > - { > - ret = unregister_vga_regions(assigned_device); > - if ( ret != 0 ) > - PT_LOG("VGA region unmapping failed\n"); > - } > + unregister_vga_regions(assigned_device); > } > > static uint8_t find_cap_offset(struct pci_dev *pci_dev, uint8_t cap) > @@ -2097,46 +2037,66 @@ static uint32_t find_ext_cap_offset(struct pci_dev *pci_dev, uint32_t cap) > return 0; > } > > -u8 pt_pci_host_read_byte(int bus, int dev, int fn, u32 addr) > +static void pci_access_init(void) > { > - struct pci_dev *pci_dev; > - u8 val; > + struct pci_access *pci_access; > > - pci_dev = pci_get_dev(dpci_infos.pci_access, 0, bus, dev, fn); > - if ( !pci_dev ) > - return 0; > + if (dpci_infos.pci_access) > + return; > > - val = pci_read_byte(pci_dev, addr); > - pci_free_dev(pci_dev); > - return val; > + /* Initialize libpci */ > + pci_access = pci_alloc(); > + if ( pci_access == NULL ) { > + PT_LOG("Error: pci_access is NULL\n"); > + return -1; > + } > + pci_init(pci_access); > + pci_scan_bus(pci_access); > + dpci_infos.pci_access = pci_access; > } > > -u16 pt_pci_host_read_word(int bus, int dev, int fn, u32 addr) > +u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len) > { > + > struct pci_dev *pci_dev; > - u16 val; > + u32 val = -1; > > + pci_access_init(); > pci_dev = pci_get_dev(dpci_infos.pci_access, 0, bus, dev, fn); > if ( !pci_dev ) > return 0; > > - val = pci_read_word(pci_dev, addr); > - pci_free_dev(pci_dev); > + switch (len) > + { > + case 1: val = pci_read_byte(pci_dev, addr); break; > + case 2: val = pci_read_word(pci_dev, addr); break; > + case 4: val = pci_read_long(pci_dev, addr); break; > + default: > + fprintf(stderr, "error: pt_pci_host_read: invalid len = %d\n", len); > + } > return val; > } > > -u32 pt_pci_host_read_long(int bus, int dev, int fn, u32 addr) > +int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len) > { > struct pci_dev *pci_dev; > - u32 val; > + int ret = 0; > > + pci_access_init(); > pci_dev = pci_get_dev(dpci_infos.pci_access, 0, bus, dev, fn); > if ( !pci_dev ) > return 0; > > - val = pci_read_long(pci_dev, addr); > + switch (len) > + { > + case 1: ret = pci_write_byte(pci_dev, addr, val); break; > + case 2: ret = pci_write_word(pci_dev, addr, val); break; > + case 4: ret = pci_write_long(pci_dev, addr, val); break; > + default: > + fprintf(stderr, "error: pt_pci_host_write: invalid len = %d\n", len); > + } > pci_free_dev(pci_dev); > - return val; > + return ret; > } > > /* parse BAR */ > @@ -4200,92 +4160,6 @@ static int pt_pmcsr_reg_restore(struct pt_dev *ptdev, > return 0; > } > > -static int get_vgabios(unsigned char *buf) > -{ > - int fd; > - uint32_t bios_size = 0; > - uint32_t start = 0xC0000; > - uint16_t magic = 0; > - > - if ( (fd = open("/dev/mem", O_RDONLY)) < 0 ) > - { > - PT_LOG("Error: Can''t open /dev/mem: %s\n", strerror(errno)); > - return 0; > - } > - > - /* > - * Check if it a real bios extension. > - * The magic number is 0xAA55. > - */ > - if ( start != lseek(fd, start, SEEK_SET) ) > - goto out; > - if ( read(fd, &magic, 2) != 2 ) > - goto out; > - if ( magic != 0xAA55 ) > - goto out; > - > - /* Find the size of the rom extension */ > - if ( start != lseek(fd, start, SEEK_SET) ) > - goto out; > - if ( lseek(fd, 2, SEEK_CUR) != (start + 2) ) > - goto out; > - if ( read(fd, &bios_size, 1) != 1 ) > - goto out; > - > - /* This size is in 512 bytes */ > - bios_size *= 512; > - > - /* > - * Set the file to the begining of the rombios, > - * to start the copy. > - */ > - if ( start != lseek(fd, start, SEEK_SET) ) > - goto out; > - > - if ( bios_size != read(fd, buf, bios_size)) > - bios_size = 0; > - > -out: > - close(fd); > - return bios_size; > -} > - > -static int setup_vga_pt(void) > -{ > - unsigned char *bios = NULL; > - int bios_size = 0; > - char *c = NULL; > - char checksum = 0; > - int rc = 0; > - > - /* Allocated 64K for the vga bios */ > - if ( !(bios = malloc(64 * 1024)) ) > - return -1; > - > - bios_size = get_vgabios(bios); > - if ( bios_size == 0 || bios_size > 64 * 1024) > - { > - PT_LOG("vga bios size (0x%x) is invalid!\n", bios_size); > - rc = -1; > - goto out; > - } > - > - /* Adjust the bios checksum */ > - for ( c = (char*)bios; c < ((char*)bios + bios_size); c++ ) > - checksum += *c; > - if ( checksum ) > - { > - bios[bios_size - 1] -= checksum; > - PT_LOG("vga bios checksum is adjusted!\n"); > - } > - > - cpu_physical_memory_rw(0xc0000, bios, bios_size, 1); > - > -out: > - free(bios); > - return rc; > -} > - > static struct pt_dev * register_real_device(PCIBus *e_bus, > const char *e_dev_name, int e_devfn, uint8_t r_bus, uint8_t r_dev, > uint8_t r_func, uint32_t machine_irq, struct pci_access *pci_access, > @@ -4387,16 +4261,13 @@ static struct pt_dev * register_real_device(PCIBus *e_bus, > pt_register_regions(assigned_device); > > /* Setup VGA bios for passthroughed gfx */ > - if ( gfx_passthru && (assigned_device->pci_dev->device_class == 0x0300) ) > + if ( setup_vga_pt(assigned_device) < 0 ) > { > - rc = setup_vga_pt(); > - if ( rc < 0 ) > - { > - PT_LOG("Setup VGA BIOS of passthroughed gfx failed!\n"); > - return NULL; > - } > + PT_LOG("Setup VGA BIOS of passthroughed gfx failed!\n"); > + return NULL; > } > > + > /* reinitialize each config register to be emulated */ > rc = pt_config_init(assigned_device); > if ( rc < 0 ) { > @@ -4548,19 +4419,8 @@ int power_on_php_devfn(int devfn) > { > struct php_dev *php_dev = &dpci_infos.php_devs[devfn]; > struct pt_dev *pt_dev; > - struct pci_access *pci_access; > > - if (!dpci_infos.pci_access) { > - /* Initialize libpci */ > - pci_access = pci_alloc(); > - if ( pci_access == NULL ) { > - PT_LOG("Error: pci_access is NULL\n"); > - return -1; > - } > - pci_init(pci_access); > - pci_scan_bus(pci_access); > - dpci_infos.pci_access = pci_access; > - } > + pci_access_init(); > > pt_dev > register_real_device(dpci_infos.e_bus, > diff --git a/hw/pass-through.h b/hw/pass-through.h > index f8a0c73..6c5e8ca 100644 > --- a/hw/pass-through.h > +++ b/hw/pass-through.h > @@ -406,10 +406,16 @@ static inline pciaddr_t pt_pci_base_addr(pciaddr_t base) > } > > uint8_t pci_intx(struct pt_dev *ptdev); > - > -u8 pt_pci_host_read_byte(int bus, int dev, int fn, u32 addr); > -u16 pt_pci_host_read_word(int bus, int dev, int fn, u32 addr); > -u32 pt_pci_host_read_long(int bus, int dev, int fn, u32 addr); > +u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len); > +int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len); > +PCIBus *intel_pch_init(PCIBus *bus); > +int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); > +int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len); > +int register_vga_regions(struct pt_dev *real_device); > +int unregister_vga_regions(struct pt_dev *real_device); > +int setup_vga_pt(struct pt_dev *real_device); > +PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, > + uint16_t did, const char *name, uint16_t revision); > > #endif /* __PASSTHROUGH_H__ */ > > diff --git a/hw/pc.c b/hw/pc.c > index 9375951..7364cb8 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -42,6 +42,10 @@ > #include "virtio-console.h" > #include "hpet_emul.h" > > +#ifdef CONFIG_PASSTHROUGH > +#include "pass-through.h" > +#endif > + > /* output Bochs bios info messages */ > //#define DEBUG_BIOS > > @@ -978,6 +982,8 @@ vga_bios_error: > pci_bus = NULL; > } > > + intel_pch_init(pci_bus); > + > /* init basic PC hardware */ > register_ioport_write(0x80, 1, 1, ioport80_write, NULL); >You are missing a ifdef CONFIG_PASSTHROUGH here> diff --git a/hw/pci.c b/hw/pci.c > index b07e5ea..fa32ed9 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -39,24 +39,6 @@ extern int igd_passthru; > > //#define DEBUG_PCI > > -struct PCIBus { > - int bus_num; > - int devfn_min; > - pci_set_irq_fn set_irq; > - pci_map_irq_fn map_irq; > - uint32_t config_reg; /* XXX: suppress */ > - /* low level pic */ > - SetIRQFunc *low_set_irq; > - qemu_irq *irq_opaque; > - PCIDevice *devices[256]; > - PCIDevice *parent_dev; > - PCIBus *next; > - /* The bus IRQ state is the logical OR of the connected devices. > - Keep a count of the number of devices with raised IRQs. */ > - int nirq; > - int irq_count[]; > -}; > - > static void pci_update_mappings(PCIDevice *d); > static void pci_set_irq(void *opaque, int irq_num, int level); > > @@ -96,7 +78,7 @@ PCIBus *pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > return bus; > } > > -static PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq) > +PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq) > { > PCIBus *bus; > bus = qemu_mallocz(sizeof(PCIBus)); > @@ -590,6 +572,10 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > printf("pci_config_write: %s: addr=%02x val=%08x len=%d\n", > pci_dev->name, config_addr, val, len); > #endif > + > +#ifdef CONFIG_PASSTHROUGH > + if (igd_pci_write(pci_dev, config_addr, val, len) == 0) > +#endif > pci_dev->config_write(pci_dev, config_addr, val, len); > } > > @@ -598,7 +584,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > PCIBus *s = opaque; > PCIDevice *pci_dev; > int config_addr, bus_num; > - uint32_t val; > + uint32_t val = 0; > > bus_num = (addr >> 16) & 0xff; > while (s && s->bus_num != bus_num) > @@ -625,26 +611,8 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > config_addr = addr & 0xff; > > #ifdef CONFIG_PASSTHROUGH > - /* host bridge reads for IGD passthrough */ > - if ( igd_passthru && pci_dev->devfn == 0x00 ) { > - val = pci_dev->config_read(pci_dev, config_addr, len); > - > - if ( config_addr == 0x00 && len == 4 ) > - val = pt_pci_host_read_long(0, 0, 0, 0x00); > - else if ( config_addr == 0x02 ) // Device ID > - val = pt_pci_host_read_word(0, 0, 0, 0x02); > - else if ( config_addr == 0x52 ) // GMCH Graphics Control Register > - val = pt_pci_host_read_word(0, 0, 0, 0x52); > - else if ( config_addr == 0xa0 ) // GMCH Top of Memory Register > - val = pt_pci_host_read_word(0, 0, 0, 0xa0); > - goto done_config_read; > - } else if ( igd_passthru && pci_dev->devfn == 0x10 && > - config_addr == 0xfc ) { // read on IGD device > - val = 0; // use SMI to communicate with the system BIOS > - goto done_config_read; > - } > + if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0) > #endif > - > val = pci_dev->config_read(pci_dev, config_addr, len); > > done_config_read: > @@ -892,12 +860,7 @@ void pci_unplug_netifs(void) > } > } > > -typedef struct { > - PCIDevice dev; > - PCIBus *bus; > -} PCIBridge; > - > -static void pci_bridge_write_config(PCIDevice *d, > +void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { > PCIBridge *s = (PCIBridge *)d; > diff --git a/hw/pci.h b/hw/pci.h > index de5a4e1..18e7b6f 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -207,6 +207,32 @@ struct PCIDevice { > int irq_state[4]; > }; > > +typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); > +typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > + > +struct PCIBus { > + int bus_num; > + int devfn_min; > + pci_set_irq_fn set_irq; > + pci_map_irq_fn map_irq; > + uint32_t config_reg; /* XXX: suppress */ > + /* low level pic */ > + SetIRQFunc *low_set_irq; > + qemu_irq *irq_opaque; > + PCIDevice *devices[256]; > + PCIDevice *parent_dev; > + PCIBus *next; > + /* The bus IRQ state is the logical OR of the connected devices. > + Keep a count of the number of devices with raised IRQs. */ > + int nirq; > + int irq_count[]; > +}; > + > +typedef struct { > + PCIDevice dev; > + PCIBus *bus; > +} PCIBridge; > + > extern char direct_pci_str[]; > extern int direct_pci_msitranslate; > extern int direct_pci_power_mgmt; > @@ -235,8 +261,6 @@ void pci_default_write_config(PCIDevice *d, > void pci_device_save(PCIDevice *s, QEMUFile *f); > int pci_device_load(PCIDevice *s, QEMUFile *f); > > -typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); > -typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > PCIBus *pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > qemu_irq *pic, int devfn_min, int nirq); > > @@ -341,5 +365,9 @@ PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > /* pass-through.c */ > int pt_init(PCIBus *e_bus); > +void pci_bridge_write_config(PCIDevice *d, > + uint32_t address, uint32_t val, int len); > +PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq); > + > > #endif > diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c > new file mode 100644 > index 0000000..3b9ce95 > --- /dev/null > +++ b/hw/pt-graphics.c > @@ -0,0 +1,266 @@ > +/* > + * graphics passthrough > + */ > + > +#include "pass-through.h" > +#include "pci/header.h" > +#include "pci/pci.h" > +#include "pt-msi.h" > +#include "qemu-xen.h" > +#include "iomulti.h" > + > +#include <unistd.h> > +#include <sys/ioctl.h> > + > +extern int gfx_passthru; > +extern int igd_passthru; > + > +static int pch_irq_function(PCIDevice *pci_dev, int irq_num) > +{ > + PT_LOG("pch_irq_function called\n"); > + return irq_num; > +} > + > +PCIBus *intel_pch_init(PCIBus *bus) > +{ > + PCIBridge *pch; > + u16 vendor_id, device_id; > + u8 rev_id; > + > + if ( !gfx_passthru ) > + return NULL; > + > + vendor_id = pt_pci_host_read(0, 0x1f, 0, 0, 2); > + device_id = pt_pci_host_read(0, 0x1f, 0, 2, 2); > + rev_id = pt_pci_host_read(0, 0x1f, 0, 8, 1); > + > + pch = (PCIBridge *) > + pci_register_device(bus, "intel_bridge_1f", sizeof(PCIBridge), > + PCI_DEVFN(0x1f, 0), NULL, pci_bridge_write_config); > + > + pci_config_set_vendor_id(pch->dev.config, vendor_id); > + pci_config_set_device_id(pch->dev.config, device_id); > + > + pch->dev.config[0x04] = 0x06; // command = bus master, pci mem > + pch->dev.config[0x05] = 0x00; > + pch->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error > + pch->dev.config[0x07] = 0x00; // status = fast devsel > + pch->dev.config[0x08] = rev_id; > + pch->dev.config[0x09] = 0x00; // programming i/f > + pci_config_set_class(pch->dev.config, PCI_CLASS_BRIDGE_ISA); > + pch->dev.config[0x0D] = 0x10; // latency_timer > + pch->dev.config[0x0E] = 0x81; // header_type > + pch->dev.config[0x1E] = 0xa0; // secondary status > + > + pch->bus = pci_register_secondary_bus(&pch->dev, pch_irq_function); > + return pch->bus; > +} > +What happens if the hardware is a pre-Calpella Intel graphic card? Is it still going to work?> +int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) > +{ > + if ( !igd_passthru || (pci_dev->devfn != 0x00 ) ) > + return 0; > + > + switch (config_addr) > + { > + case 0x58: // PAVPC Offset > + pt_pci_host_write(0, 0, 0, config_addr, val, len); > + PT_LOG("pci_config_write: %x:%x.%x: addr=%x len=%x val=%x\n", > + pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn), config_addr, len, val); > + break; > + default: > + pci_dev->config_write(pci_dev, config_addr, val, len); > + } > + return 1; > +} > + > +int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) > +{ > + if ( !igd_passthru || (pci_dev->devfn != 0) ) > + return 0; > + > + switch (config_addr) > + { > + case 0x00: /* vendor id */ > + case 0x02: /* device id */ > + case 0x52: /* processor graphics control register */ > + case 0xa0: /* top of memory */ > + case 0xb0: /* ILK: BSM: should read from dev 2 offset 0x5c */ > + case 0x58: /* SNB: PAVPC Offset */ > + case 0xa4: /* SNB: graphics base of stolen memory */ > + case 0xa8: /* SNB: base of GTT stolen memory */ > + *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), > + 0, config_addr, len); > + PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n", > + pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn), config_addr, len, *val); > + > + break; > + default: > + *val = pci_dev->config_read(pci_dev, config_addr, len); > + } > + return 1; > +} > + > +/* > + * register VGA resources for the domain with assigned gfx > + */ > +int register_vga_regions(struct pt_dev *real_device) > +{ > + u32 igd_opregion, igd_bsm; > + int ret = 0; > + > + if ( !gfx_passthru || real_device->pci_dev->device_class != 0x0300 ) > + return ret; > + > + ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3B0, > + 0x3B0, 0xC, DPCI_ADD_MAPPING); > + > + ret |= xc_domain_ioport_mapping(xc_handle, domid, 0x3C0, > + 0x3C0, 0x20, DPCI_ADD_MAPPING); > + > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + 0xa0000 >> XC_PAGE_SHIFT, > + 0xa0000 >> XC_PAGE_SHIFT, > + 0x20, > + DPCI_ADD_MAPPING); > + > + /* 1:1 map ASL Storage register value */ > + igd_opregion = pt_pci_host_read(0, 2, 0, 0xfc, 4); > + PT_LOG("register_vga: igd_opregion = %x\n", igd_opregion); > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + igd_opregion >> XC_PAGE_SHIFT, > + igd_opregion >> XC_PAGE_SHIFT, > + 2, > + DPCI_ADD_MAPPING);Again, what happens if the hardware is older? Do all Intel graphic cards have an opregion?> + igd_opregion = pt_pci_host_read(0, 2, 0, 0xfc, 4); > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + igd_opregion >> XC_PAGE_SHIFT, > + igd_opregion >> XC_PAGE_SHIFT, > + 2, > + DPCI_REMOVE_MAPPING); > +ditto _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-09 15:57 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Tue, 8 Jun 2010, Kay, Allen M wrote:> Stephano, > > Thanks for your feedback. I add "#ifdef CONFIG_PASSTHROUGH" in the next version of the patch. > > I have tested the patch on a Intel Montevina software development platform and found it has the same behavior as current xen qemu upstream. > > Can others who have been playing with IGD passthrough give the patch a try on older platforms? >Are there any older platforms without opregion support? If that is the case what is the result of executing this code on such a platform? + /* 1:1 map ASL Storage register value */ + igd_opregion = pt_pci_host_read(0, 2, 0, 0xfc, 4); + PT_LOG("register_vga: igd_opregion = %x\n", igd_opregion); + ret |= xc_domain_memory_mapping(xc_handle, domid, + igd_opregion >> XC_PAGE_SHIFT, + igd_opregion >> XC_PAGE_SHIFT, + 2, + DPCI_ADD_MAPPING); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-10 13:21 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Tue, 8 Jun 2010, Kay, Allen M wrote:> Doing this resulted in a lot of duplicated code and also force code path to change > even when IGD passthrough is not used. To do it correctly, I also need to > put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep > the original patch.Can you register a dummy device at the address corresponding to IGD and get the pci conf read and write calls directly from the functions you pass to pci_register_device? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2010-Jun-10 14:17 UTC
Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On 9 June 2010 16:57, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> On Tue, 8 Jun 2010, Kay, Allen M wrote: >> Stephano, >> >> Thanks for your feedback. I add "#ifdef CONFIG_PASSTHROUGH" in the next version of the patch. >> >> I have tested the patch on a Intel Montevina software development platform and found it has the same behavior as current xen qemu upstream. >> >> Can others who have been playing with IGD passthrough give the patch a try on older platforms? >> > > Are there any older platforms without opregion support? > If that is the case what is the result of executing this code on such a > platform? > > + /* 1:1 map ASL Storage register value */ > + igd_opregion = pt_pci_host_read(0, 2, 0, 0xfc, 4); > + PT_LOG("register_vga: igd_opregion = %x\n", igd_opregion); > + ret |= xc_domain_memory_mapping(xc_handle, domid, > + igd_opregion >> XC_PAGE_SHIFT, > + igd_opregion >> XC_PAGE_SHIFT, > + 2, > + DPCI_ADD_MAPPING); > >We are using a fixed and reserved address in the guest to map this region, 0xfed00000. You need to add that in the e820 table as ACPI_NVS so the guest won''t try to use it for something else. You also have to trap the pci_config space read for 0xfc and give 0xfed00000 back to the guest. We''ve notice that on some system the value at 0xfc wasn''t align on a page, so make sure you do that first. Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2010-Jun-10 16:46 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
> Can you register a dummy device at the address corresponding to IGD and > get the pci conf read and write calls directly from the functions you > pass to pci_register_device?I''m not sure what do you mean... The issue is IGD driver accesses some registers in device 0:0.0. However, we don''t want to passthrough platform''s 0:0.0 to the guest as it will change the guest''s chipset wholesale thus create more problems than it solves. For now, we are keeping the 440 chipset''s 0:0.0 device but passthrough certain register accesses. Isaku proposed register another set of read/write function for guest''s device 0:0.0 instead of modifying existing functions. The advantage of this approach is that original QEMU read/write function will be left unmodified for IGD purpose. The disadvantage is this requires making a copy of exiting code and redirect all read/write handling of 0:0.0 device to a function in pt-graphics.c. I tried it out but found the resulting code a bit confusing. Allen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-11 11:08 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Tue, 8 Jun 2010, Kay, Allen M wrote:> Isaku, > > Thanks for the feedback. > > > pci_{read, write}_block() would be better than > > switch(len) case 1: case 2: case4:... > > Done! > > > Is it really necessary to move PCIBus and PCIBridge to the header file? > > Doesn''t pci_bridge_init() work? > > I changed to code to utilize pci_bridge_init(). However, I still need to move > PCIBus and PCIBridge defines to the header file. The alternative is to pollute > pc.c with graphics passthrough specific code. > > > Overriding pci config read/write methods of i440fx would be much cleaner > > than hooking pci_data_read/write. (pass igd_pci_read/write to > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL) > > Doing this resulted in a lot of duplicated code and also force code path to change > even when IGD passthrough is not used. To do it correctly, I also need to > put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep > the original patch. >I see. Even though the patch is an improvement over what we currently have in qemu-xen, it is still not acceptable in upstream qemu as it is. Considering that we are pretty close to have the merge complete, I think it worth trying to come up with something cleaner. Isaku, you are the latest one to touch the pci_host code in qemu, do you have any other suggestion? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Isaku Yamahata
2010-Jun-14 03:30 UTC
Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Fri, Jun 11, 2010 at 12:08:29PM +0100, Stefano Stabellini wrote:> On Tue, 8 Jun 2010, Kay, Allen M wrote: > > Isaku, > > > > Thanks for the feedback. > > > > > pci_{read, write}_block() would be better than > > > switch(len) case 1: case 2: case4:... > > > > Done! > > > > > Is it really necessary to move PCIBus and PCIBridge to the header file? > > > Doesn''t pci_bridge_init() work? > > > > I changed to code to utilize pci_bridge_init(). However, I still need to move > > PCIBus and PCIBridge defines to the header file. The alternative is to pollute > > pc.c with graphics passthrough specific code. > > > > > Overriding pci config read/write methods of i440fx would be much cleaner > > > than hooking pci_data_read/write. (pass igd_pci_read/write to > > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL) > > > > Doing this resulted in a lot of duplicated code and also force code path to change > > even when IGD passthrough is not used. To do it correctly, I also need to > > put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep > > the original patch. > > > > I see. > > Even though the patch is an improvement over what we currently have in > qemu-xen, it is still not acceptable in upstream qemu as it is. > Considering that we are pretty close to have the merge complete, I think > it worth trying to come up with something cleaner. > > Isaku, you are the latest one to touch the pci_host code in qemu, do you > have any other suggestion?How about the following patch which is on top of the v2 patch? I did only compile test, but I think it''s enough to show my idea. For PCI bridge, I have a patches to clean up/enhance the implementation and I''m planning to push to the qemu upstream soon.>From 9a22525e523e834ab3822d14e715388c19a7d02e Mon Sep 17 00:00:00 2001Message-Id: <9a22525e523e834ab3822d14e715388c19a7d02e.1276485897.git.yamahata@valinux.co.jp> In-Reply-To: <cover.1276485897.git.yamahata@valinux.co.jp> References: <cover.1276485897.git.yamahata@valinux.co.jp> From: Isaku Yamahata <yamahata@valinux.co.jp> Date: Mon, 14 Jun 2010 12:24:31 +0900 Subject: [PATCH] pci passthrough: some clean up and unexport PCIBus and PCIBridge. some clean up and unexport PCIBus and PCIBridge. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pass-through.h | 10 ++++++++-- hw/pci.c | 29 +++++++++++++++++++++++------ hw/pci.h | 23 ----------------------- hw/piix_pci.c | 4 ++-- hw/pt-graphics.c | 32 +++++++++++++++++++------------- 5 files changed, 52 insertions(+), 46 deletions(-) diff --git a/hw/pass-through.h b/hw/pass-through.h index 242d079..adc9f58 100644 --- a/hw/pass-through.h +++ b/hw/pass-through.h @@ -409,13 +409,19 @@ uint8_t pci_intx(struct pt_dev *ptdev); u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len); int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len); void intel_pch_init(PCIBus *bus); -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len); int register_vga_regions(struct pt_dev *real_device); int unregister_vga_regions(struct pt_dev *real_device); int setup_vga_pt(struct pt_dev *real_device); PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, const char *name, uint16_t revision); +#ifdef CONFIG_PASSTHROUGH +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len); +#else +#define igd_pci_write(pci_dev, config_addr, val, len) NULL +#define igd_pci_read(pci_dev, config_addr, val, len) NULL +#endif + #endif /* __PASSTHROUGH_H__ */ diff --git a/hw/pci.c b/hw/pci.c index a5cb378..b6400f3 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -39,6 +39,24 @@ extern int igd_passthru; //#define DEBUG_PCI +struct PCIBus { + int bus_num; + int devfn_min; + pci_set_irq_fn set_irq; + pci_map_irq_fn map_irq; + uint32_t config_reg; /* XXX: suppress */ + /* low level pic */ + SetIRQFunc *low_set_irq; + qemu_irq *irq_opaque; + PCIDevice *devices[256]; + PCIDevice *parent_dev; + PCIBus *next; + /* The bus IRQ state is the logical OR of the connected devices. + Keep a count of the number of devices with raised IRQs. */ + int nirq; + int irq_count[]; +}; + static void pci_update_mappings(PCIDevice *d); static void pci_set_irq(void *opaque, int irq_num, int level); @@ -573,9 +591,6 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) pci_dev->name, config_addr, val, len); #endif -#ifdef CONFIG_PASSTHROUGH - if (igd_pci_write(pci_dev, config_addr, val, len) == 0) -#endif pci_dev->config_write(pci_dev, config_addr, val, len); } @@ -610,9 +625,6 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) } config_addr = addr & 0xff; -#ifdef CONFIG_PASSTHROUGH - if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0) -#endif val = pci_dev->config_read(pci_dev, config_addr, len); done_config_read: @@ -860,6 +872,11 @@ void pci_unplug_netifs(void) } } +typedef struct { + PCIDevice dev; + PCIBus *bus; +} PCIBridge; + void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { diff --git a/hw/pci.h b/hw/pci.h index 7a44035..8abbad7 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -210,29 +210,6 @@ struct PCIDevice { typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); -struct PCIBus { - int bus_num; - int devfn_min; - pci_set_irq_fn set_irq; - pci_map_irq_fn map_irq; - uint32_t config_reg; /* XXX: suppress */ - /* low level pic */ - SetIRQFunc *low_set_irq; - qemu_irq *irq_opaque; - PCIDevice *devices[256]; - PCIDevice *parent_dev; - PCIBus *next; - /* The bus IRQ state is the logical OR of the connected devices. - Keep a count of the number of devices with raised IRQs. */ - int nirq; - int irq_count[]; -}; - -typedef struct { - PCIDevice dev; - PCIBus *bus; -} PCIBridge; - extern char direct_pci_str[]; extern int direct_pci_msitranslate; extern int direct_pci_power_mgmt; diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 49d72b2..1bfe0fb 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -25,7 +25,7 @@ #include "hw.h" #include "pc.h" #include "pci.h" - +#include "pass-through.h" static void i440fx_set_irq(qemu_irq *pic, int irq_num, int level); static void piix3_write_config(PCIDevice *d, @@ -207,7 +207,7 @@ PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic) register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s); d = pci_register_device(b, "i440FX", sizeof(PCIDevice), 0, - NULL, NULL); + igd_pci_read, igd_pci_write); pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_INTEL); pci_config_set_device_id(d->config, PCI_DEVICE_ID_INTEL_82441); diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c index 4923715..32c174d 100644 --- a/hw/pt-graphics.c +++ b/hw/pt-graphics.c @@ -8,6 +8,7 @@ #include <unistd.h> #include <sys/ioctl.h> +#include <assert.h> extern int gfx_passthru; extern int igd_passthru; @@ -34,10 +35,13 @@ void intel_pch_init(PCIBus *bus) pch_map_irq, "intel_bridge_1f"); } -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) { - if ( !igd_passthru || (pci_dev->devfn != 0x00 ) ) - return 0; + assert(pci_dev->devfn == 0x00); + if ( !igd_passthru ) { + pci_default_write_config(pci_dev, config_addr, val, len); + return; + } switch (config_addr) { @@ -48,15 +52,18 @@ int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) PCI_FUNC(pci_dev->devfn), config_addr, len, val); break; default: - pci_dev->config_write(pci_dev, config_addr, val, len); + pci_default_write_config(pci_dev, config_addr, val, len); } - return 1; } -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len) { - if ( !igd_passthru || (pci_dev->devfn != 0) ) - return 0; + uint32_t val; + + assert(pci_dev->devfn == 0x00); + if ( !igd_passthru ) { + return pci_default_read_config(pci_dev, config_addr, len); + } switch (config_addr) { @@ -68,17 +75,16 @@ int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) case 0x58: /* SNB: PAVPC Offset */ case 0xa4: /* SNB: graphics base of stolen memory */ case 0xa8: /* SNB: base of GTT stolen memory */ - *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), + val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), 0, config_addr, len); PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n", pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), - PCI_FUNC(pci_dev->devfn), config_addr, len, *val); - + PCI_FUNC(pci_dev->devfn), config_addr, len, val); break; default: - *val = pci_dev->config_read(pci_dev, config_addr, len); + val = pci_default_read_config(pci_dev, config_addr, len); } - return 1; + return val; } /* -- 1.6.6.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-14 11:02 UTC
Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Mon, 14 Jun 2010, Isaku Yamahata wrote:> On Fri, Jun 11, 2010 at 12:08:29PM +0100, Stefano Stabellini wrote: > > On Tue, 8 Jun 2010, Kay, Allen M wrote: > > > Isaku, > > > > > > Thanks for the feedback. > > > > > > > pci_{read, write}_block() would be better than > > > > switch(len) case 1: case 2: case4:... > > > > > > Done! > > > > > > > Is it really necessary to move PCIBus and PCIBridge to the header file? > > > > Doesn''t pci_bridge_init() work? > > > > > > I changed to code to utilize pci_bridge_init(). However, I still need to move > > > PCIBus and PCIBridge defines to the header file. The alternative is to pollute > > > pc.c with graphics passthrough specific code. > > > > > > > Overriding pci config read/write methods of i440fx would be much cleaner > > > > than hooking pci_data_read/write. (pass igd_pci_read/write to > > > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL) > > > > > > Doing this resulted in a lot of duplicated code and also force code path to change > > > even when IGD passthrough is not used. To do it correctly, I also need to > > > put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep > > > the original patch. > > > > > > > I see. > > > > Even though the patch is an improvement over what we currently have in > > qemu-xen, it is still not acceptable in upstream qemu as it is. > > Considering that we are pretty close to have the merge complete, I think > > it worth trying to come up with something cleaner. > > > > Isaku, you are the latest one to touch the pci_host code in qemu, do you > > have any other suggestion? > > How about the following patch which is on top of the v2 patch? > I did only compile test, but I think it''s enough to show my idea. > > For PCI bridge, I have a patches to clean up/enhance the implementation and > I''m planning to push to the qemu upstream soon. >I think this looks much better, thanks Isaku! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2010-Jun-16 00:58 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
Isaku, I cannot apply the patch below because format problems. Can you resend it as a attachment? Allen -----Original Message----- From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Isaku Yamahata Sent: Sunday, June 13, 2010 8:30 PM To: Stefano Stabellini Cc: xen-devel@lists.xensource.com; Kay, Allen M; Han, Weidong; Jean Guyader; Ian Pratt; Ross Philipson Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge On Fri, Jun 11, 2010 at 12:08:29PM +0100, Stefano Stabellini wrote:> On Tue, 8 Jun 2010, Kay, Allen M wrote: > > Isaku, > > > > Thanks for the feedback. > > > > > pci_{read, write}_block() would be better than > > > switch(len) case 1: case 2: case4:... > > > > Done! > > > > > Is it really necessary to move PCIBus and PCIBridge to the header file? > > > Doesn''t pci_bridge_init() work? > > > > I changed to code to utilize pci_bridge_init(). However, I still need to move > > PCIBus and PCIBridge defines to the header file. The alternative is to pollute > > pc.c with graphics passthrough specific code. > > > > > Overriding pci config read/write methods of i440fx would be much cleaner > > > than hooking pci_data_read/write. (pass igd_pci_read/write to > > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL) > > > > Doing this resulted in a lot of duplicated code and also force code path to change > > even when IGD passthrough is not used. To do it correctly, I also need to > > put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep > > the original patch. > > > > I see. > > Even though the patch is an improvement over what we currently have in > qemu-xen, it is still not acceptable in upstream qemu as it is. > Considering that we are pretty close to have the merge complete, I think > it worth trying to come up with something cleaner. > > Isaku, you are the latest one to touch the pci_host code in qemu, do you > have any other suggestion?How about the following patch which is on top of the v2 patch? I did only compile test, but I think it''s enough to show my idea. For PCI bridge, I have a patches to clean up/enhance the implementation and I''m planning to push to the qemu upstream soon.>From 9a22525e523e834ab3822d14e715388c19a7d02e Mon Sep 17 00:00:00 2001Message-Id: <9a22525e523e834ab3822d14e715388c19a7d02e.1276485897.git.yamahata@valinux.co.jp> In-Reply-To: <cover.1276485897.git.yamahata@valinux.co.jp> References: <cover.1276485897.git.yamahata@valinux.co.jp> From: Isaku Yamahata <yamahata@valinux.co.jp> Date: Mon, 14 Jun 2010 12:24:31 +0900 Subject: [PATCH] pci passthrough: some clean up and unexport PCIBus and PCIBridge. some clean up and unexport PCIBus and PCIBridge. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pass-through.h | 10 ++++++++-- hw/pci.c | 29 +++++++++++++++++++++++------ hw/pci.h | 23 ----------------------- hw/piix_pci.c | 4 ++-- hw/pt-graphics.c | 32 +++++++++++++++++++------------- 5 files changed, 52 insertions(+), 46 deletions(-) diff --git a/hw/pass-through.h b/hw/pass-through.h index 242d079..adc9f58 100644 --- a/hw/pass-through.h +++ b/hw/pass-through.h @@ -409,13 +409,19 @@ uint8_t pci_intx(struct pt_dev *ptdev); u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len); int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len); void intel_pch_init(PCIBus *bus); -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len); int register_vga_regions(struct pt_dev *real_device); int unregister_vga_regions(struct pt_dev *real_device); int setup_vga_pt(struct pt_dev *real_device); PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, const char *name, uint16_t revision); +#ifdef CONFIG_PASSTHROUGH +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len); +#else +#define igd_pci_write(pci_dev, config_addr, val, len) NULL +#define igd_pci_read(pci_dev, config_addr, val, len) NULL +#endif + #endif /* __PASSTHROUGH_H__ */ diff --git a/hw/pci.c b/hw/pci.c index a5cb378..b6400f3 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -39,6 +39,24 @@ extern int igd_passthru; //#define DEBUG_PCI +struct PCIBus { + int bus_num; + int devfn_min; + pci_set_irq_fn set_irq; + pci_map_irq_fn map_irq; + uint32_t config_reg; /* XXX: suppress */ + /* low level pic */ + SetIRQFunc *low_set_irq; + qemu_irq *irq_opaque; + PCIDevice *devices[256]; + PCIDevice *parent_dev; + PCIBus *next; + /* The bus IRQ state is the logical OR of the connected devices. + Keep a count of the number of devices with raised IRQs. */ + int nirq; + int irq_count[]; +}; + static void pci_update_mappings(PCIDevice *d); static void pci_set_irq(void *opaque, int irq_num, int level); @@ -573,9 +591,6 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) pci_dev->name, config_addr, val, len); #endif -#ifdef CONFIG_PASSTHROUGH - if (igd_pci_write(pci_dev, config_addr, val, len) == 0) -#endif pci_dev->config_write(pci_dev, config_addr, val, len); } @@ -610,9 +625,6 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) } config_addr = addr & 0xff; -#ifdef CONFIG_PASSTHROUGH - if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0) -#endif val = pci_dev->config_read(pci_dev, config_addr, len); done_config_read: @@ -860,6 +872,11 @@ void pci_unplug_netifs(void) } } +typedef struct { + PCIDevice dev; + PCIBus *bus; +} PCIBridge; + void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { diff --git a/hw/pci.h b/hw/pci.h index 7a44035..8abbad7 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -210,29 +210,6 @@ struct PCIDevice { typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); -struct PCIBus { - int bus_num; - int devfn_min; - pci_set_irq_fn set_irq; - pci_map_irq_fn map_irq; - uint32_t config_reg; /* XXX: suppress */ - /* low level pic */ - SetIRQFunc *low_set_irq; - qemu_irq *irq_opaque; - PCIDevice *devices[256]; - PCIDevice *parent_dev; - PCIBus *next; - /* The bus IRQ state is the logical OR of the connected devices. - Keep a count of the number of devices with raised IRQs. */ - int nirq; - int irq_count[]; -}; - -typedef struct { - PCIDevice dev; - PCIBus *bus; -} PCIBridge; - extern char direct_pci_str[]; extern int direct_pci_msitranslate; extern int direct_pci_power_mgmt; diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 49d72b2..1bfe0fb 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -25,7 +25,7 @@ #include "hw.h" #include "pc.h" #include "pci.h" - +#include "pass-through.h" static void i440fx_set_irq(qemu_irq *pic, int irq_num, int level); static void piix3_write_config(PCIDevice *d, @@ -207,7 +207,7 @@ PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic) register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s); d = pci_register_device(b, "i440FX", sizeof(PCIDevice), 0, - NULL, NULL); + igd_pci_read, igd_pci_write); pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_INTEL); pci_config_set_device_id(d->config, PCI_DEVICE_ID_INTEL_82441); diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c index 4923715..32c174d 100644 --- a/hw/pt-graphics.c +++ b/hw/pt-graphics.c @@ -8,6 +8,7 @@ #include <unistd.h> #include <sys/ioctl.h> +#include <assert.h> extern int gfx_passthru; extern int igd_passthru; @@ -34,10 +35,13 @@ void intel_pch_init(PCIBus *bus) pch_map_irq, "intel_bridge_1f"); } -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) { - if ( !igd_passthru || (pci_dev->devfn != 0x00 ) ) - return 0; + assert(pci_dev->devfn == 0x00); + if ( !igd_passthru ) { + pci_default_write_config(pci_dev, config_addr, val, len); + return; + } switch (config_addr) { @@ -48,15 +52,18 @@ int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) PCI_FUNC(pci_dev->devfn), config_addr, len, val); break; default: - pci_dev->config_write(pci_dev, config_addr, val, len); + pci_default_write_config(pci_dev, config_addr, val, len); } - return 1; } -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len) { - if ( !igd_passthru || (pci_dev->devfn != 0) ) - return 0; + uint32_t val; + + assert(pci_dev->devfn == 0x00); + if ( !igd_passthru ) { + return pci_default_read_config(pci_dev, config_addr, len); + } switch (config_addr) { @@ -68,17 +75,16 @@ int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) case 0x58: /* SNB: PAVPC Offset */ case 0xa4: /* SNB: graphics base of stolen memory */ case 0xa8: /* SNB: base of GTT stolen memory */ - *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), + val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), 0, config_addr, len); PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n", pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), - PCI_FUNC(pci_dev->devfn), config_addr, len, *val); - + PCI_FUNC(pci_dev->devfn), config_addr, len, val); break; default: - *val = pci_dev->config_read(pci_dev, config_addr, len); + val = pci_default_read_config(pci_dev, config_addr, len); } - return 1; + return val; } /* -- 1.6.6.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Isaku Yamahata
2010-Jun-16 02:08 UTC
Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
Attached. On Tue, Jun 15, 2010 at 05:58:57PM -0700, Kay, Allen M wrote:> Isaku, > > I cannot apply the patch below because format problems. Can you resend it as a attachment? > > Allen > > -----Original Message----- > From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Isaku Yamahata > Sent: Sunday, June 13, 2010 8:30 PM > To: Stefano Stabellini > Cc: xen-devel@lists.xensource.com; Kay, Allen M; Han, Weidong; Jean Guyader; Ian Pratt; Ross Philipson > Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge > > On Fri, Jun 11, 2010 at 12:08:29PM +0100, Stefano Stabellini wrote: > > On Tue, 8 Jun 2010, Kay, Allen M wrote: > > > Isaku, > > > > > > Thanks for the feedback. > > > > > > > pci_{read, write}_block() would be better than > > > > switch(len) case 1: case 2: case4:... > > > > > > Done! > > > > > > > Is it really necessary to move PCIBus and PCIBridge to the header file? > > > > Doesn''t pci_bridge_init() work? > > > > > > I changed to code to utilize pci_bridge_init(). However, I still need to move > > > PCIBus and PCIBridge defines to the header file. The alternative is to pollute > > > pc.c with graphics passthrough specific code. > > > > > > > Overriding pci config read/write methods of i440fx would be much cleaner > > > > than hooking pci_data_read/write. (pass igd_pci_read/write to > > > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead of NULL) > > > > > > Doing this resulted in a lot of duplicated code and also force code path to change > > > even when IGD passthrough is not used. To do it correctly, I also need to > > > put in IGD or PASSTHROGH awareness in piix_pci.c. For now, I''m going to keep > > > the original patch. > > > > > > > I see. > > > > Even though the patch is an improvement over what we currently have in > > qemu-xen, it is still not acceptable in upstream qemu as it is. > > Considering that we are pretty close to have the merge complete, I think > > it worth trying to come up with something cleaner. > > > > Isaku, you are the latest one to touch the pci_host code in qemu, do you > > have any other suggestion? > > How about the following patch which is on top of the v2 patch? > I did only compile test, but I think it''s enough to show my idea. > > For PCI bridge, I have a patches to clean up/enhance the implementation and > I''m planning to push to the qemu upstream soon. > > > >From 9a22525e523e834ab3822d14e715388c19a7d02e Mon Sep 17 00:00:00 2001 > Message-Id: <9a22525e523e834ab3822d14e715388c19a7d02e.1276485897.git.yamahata@valinux.co.jp> > In-Reply-To: <cover.1276485897.git.yamahata@valinux.co.jp> > References: <cover.1276485897.git.yamahata@valinux.co.jp> > From: Isaku Yamahata <yamahata@valinux.co.jp> > Date: Mon, 14 Jun 2010 12:24:31 +0900 > Subject: [PATCH] pci passthrough: some clean up and unexport PCIBus and PCIBridge. > > some clean up and unexport PCIBus and PCIBridge. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/pass-through.h | 10 ++++++++-- > hw/pci.c | 29 +++++++++++++++++++++++------ > hw/pci.h | 23 ----------------------- > hw/piix_pci.c | 4 ++-- > hw/pt-graphics.c | 32 +++++++++++++++++++------------- > 5 files changed, 52 insertions(+), 46 deletions(-) > > diff --git a/hw/pass-through.h b/hw/pass-through.h > index 242d079..adc9f58 100644 > --- a/hw/pass-through.h > +++ b/hw/pass-through.h > @@ -409,13 +409,19 @@ uint8_t pci_intx(struct pt_dev *ptdev); > u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len); > int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int len); > void intel_pch_init(PCIBus *bus); > -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); > -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len); > int register_vga_regions(struct pt_dev *real_device); > int unregister_vga_regions(struct pt_dev *real_device); > int setup_vga_pt(struct pt_dev *real_device); > PCIBus *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, > uint16_t did, const char *name, uint16_t revision); > > +#ifdef CONFIG_PASSTHROUGH > +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len); > +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len); > +#else > +#define igd_pci_write(pci_dev, config_addr, val, len) NULL > +#define igd_pci_read(pci_dev, config_addr, val, len) NULL > +#endif > + > #endif /* __PASSTHROUGH_H__ */ > > diff --git a/hw/pci.c b/hw/pci.c > index a5cb378..b6400f3 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -39,6 +39,24 @@ extern int igd_passthru; > > //#define DEBUG_PCI > > +struct PCIBus { > + int bus_num; > + int devfn_min; > + pci_set_irq_fn set_irq; > + pci_map_irq_fn map_irq; > + uint32_t config_reg; /* XXX: suppress */ > + /* low level pic */ > + SetIRQFunc *low_set_irq; > + qemu_irq *irq_opaque; > + PCIDevice *devices[256]; > + PCIDevice *parent_dev; > + PCIBus *next; > + /* The bus IRQ state is the logical OR of the connected devices. > + Keep a count of the number of devices with raised IRQs. */ > + int nirq; > + int irq_count[]; > +}; > + > static void pci_update_mappings(PCIDevice *d); > static void pci_set_irq(void *opaque, int irq_num, int level); > > @@ -573,9 +591,6 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > pci_dev->name, config_addr, val, len); > #endif > > -#ifdef CONFIG_PASSTHROUGH > - if (igd_pci_write(pci_dev, config_addr, val, len) == 0) > -#endif > pci_dev->config_write(pci_dev, config_addr, val, len); > } > > @@ -610,9 +625,6 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > } > config_addr = addr & 0xff; > > -#ifdef CONFIG_PASSTHROUGH > - if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0) > -#endif > val = pci_dev->config_read(pci_dev, config_addr, len); > > done_config_read: > @@ -860,6 +872,11 @@ void pci_unplug_netifs(void) > } > } > > +typedef struct { > + PCIDevice dev; > + PCIBus *bus; > +} PCIBridge; > + > void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { > diff --git a/hw/pci.h b/hw/pci.h > index 7a44035..8abbad7 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -210,29 +210,6 @@ struct PCIDevice { > typedef void (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > > -struct PCIBus { > - int bus_num; > - int devfn_min; > - pci_set_irq_fn set_irq; > - pci_map_irq_fn map_irq; > - uint32_t config_reg; /* XXX: suppress */ > - /* low level pic */ > - SetIRQFunc *low_set_irq; > - qemu_irq *irq_opaque; > - PCIDevice *devices[256]; > - PCIDevice *parent_dev; > - PCIBus *next; > - /* The bus IRQ state is the logical OR of the connected devices. > - Keep a count of the number of devices with raised IRQs. */ > - int nirq; > - int irq_count[]; > -}; > - > -typedef struct { > - PCIDevice dev; > - PCIBus *bus; > -} PCIBridge; > - > extern char direct_pci_str[]; > extern int direct_pci_msitranslate; > extern int direct_pci_power_mgmt; > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index 49d72b2..1bfe0fb 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -25,7 +25,7 @@ > #include "hw.h" > #include "pc.h" > #include "pci.h" > - > +#include "pass-through.h" > > static void i440fx_set_irq(qemu_irq *pic, int irq_num, int level); > static void piix3_write_config(PCIDevice *d, > @@ -207,7 +207,7 @@ PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic) > register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s); > > d = pci_register_device(b, "i440FX", sizeof(PCIDevice), 0, > - NULL, NULL); > + igd_pci_read, igd_pci_write); > > pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_INTEL); > pci_config_set_device_id(d->config, PCI_DEVICE_ID_INTEL_82441); > diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c > index 4923715..32c174d 100644 > --- a/hw/pt-graphics.c > +++ b/hw/pt-graphics.c > @@ -8,6 +8,7 @@ > > #include <unistd.h> > #include <sys/ioctl.h> > +#include <assert.h> > > extern int gfx_passthru; > extern int igd_passthru; > @@ -34,10 +35,13 @@ void intel_pch_init(PCIBus *bus) > pch_map_irq, "intel_bridge_1f"); > } > > -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) > +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) > { > - if ( !igd_passthru || (pci_dev->devfn != 0x00 ) ) > - return 0; > + assert(pci_dev->devfn == 0x00); > + if ( !igd_passthru ) { > + pci_default_write_config(pci_dev, config_addr, val, len); > + return; > + } > > switch (config_addr) > { > @@ -48,15 +52,18 @@ int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) > PCI_FUNC(pci_dev->devfn), config_addr, len, val); > break; > default: > - pci_dev->config_write(pci_dev, config_addr, val, len); > + pci_default_write_config(pci_dev, config_addr, val, len); > } > - return 1; > } > > -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) > +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len) > { > - if ( !igd_passthru || (pci_dev->devfn != 0) ) > - return 0; > + uint32_t val; > + > + assert(pci_dev->devfn == 0x00); > + if ( !igd_passthru ) { > + return pci_default_read_config(pci_dev, config_addr, len); > + } > > switch (config_addr) > { > @@ -68,17 +75,16 @@ int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) > case 0x58: /* SNB: PAVPC Offset */ > case 0xa4: /* SNB: graphics base of stolen memory */ > case 0xa8: /* SNB: base of GTT stolen memory */ > - *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), > + val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), > 0, config_addr, len); > PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n", > pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), > - PCI_FUNC(pci_dev->devfn), config_addr, len, *val); > - > + PCI_FUNC(pci_dev->devfn), config_addr, len, val); > break; > default: > - *val = pci_dev->config_read(pci_dev, config_addr, len); > + val = pci_default_read_config(pci_dev, config_addr, len); > } > - return 1; > + return val; > } > > /* > -- > 1.6.6.1 > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >-- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2010-Jun-18 00:01 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
Isaku/Stafano, Just want to report that this patch worked without any problems. As long as everyone is OK with hooking igd_pci_read/write functions in piix_pci.c, I''m fine with it too. I wasn''t sure about it earlier ... Allen -----Original Message----- From: Isaku Yamahata [mailto:yamahata@valinux.co.jp] Sent: Tuesday, June 15, 2010 7:08 PM To: Kay, Allen M Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Han, Weidong; Jean Guyader; Ian Pratt; Ross Philipson Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge Attached. On Tue, Jun 15, 2010 at 05:58:57PM -0700, Kay, Allen M wrote:> Isaku, > > I cannot apply the patch below because format problems. Can you resend it as a attachment? > > Allen > > -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Isaku > Yamahata > Sent: Sunday, June 13, 2010 8:30 PM > To: Stefano Stabellini > Cc: xen-devel@lists.xensource.com; Kay, Allen M; Han, Weidong; Jean > Guyader; Ian Pratt; Ross Philipson > Subject: Re: [Xen-devel] [PATCH][VTD] enable integrated graphics > passthrough for Calpella and Sandybridge > > On Fri, Jun 11, 2010 at 12:08:29PM +0100, Stefano Stabellini wrote: > > On Tue, 8 Jun 2010, Kay, Allen M wrote: > > > Isaku, > > > > > > Thanks for the feedback. > > > > > > > pci_{read, write}_block() would be better than > > > > switch(len) case 1: case 2: case4:... > > > > > > Done! > > > > > > > Is it really necessary to move PCIBus and PCIBridge to the header file? > > > > Doesn''t pci_bridge_init() work? > > > > > > I changed to code to utilize pci_bridge_init(). However, I still > > > need to move PCIBus and PCIBridge defines to the header file. The > > > alternative is to pollute pc.c with graphics passthrough specific code. > > > > > > > Overriding pci config read/write methods of i440fx would be much > > > > cleaner than hooking pci_data_read/write. (pass > > > > igd_pci_read/write to > > > > pci_register_device() in i440fx_init() in hw/piix_pci.c instead > > > > of NULL) > > > > > > Doing this resulted in a lot of duplicated code and also force > > > code path to change even when IGD passthrough is not used. To do > > > it correctly, I also need to put in IGD or PASSTHROGH awareness in > > > piix_pci.c. For now, I''m going to keep the original patch. > > > > > > > I see. > > > > Even though the patch is an improvement over what we currently have > > in qemu-xen, it is still not acceptable in upstream qemu as it is. > > Considering that we are pretty close to have the merge complete, I > > think it worth trying to come up with something cleaner. > > > > Isaku, you are the latest one to touch the pci_host code in qemu, do > > you have any other suggestion? > > How about the following patch which is on top of the v2 patch? > I did only compile test, but I think it''s enough to show my idea. > > For PCI bridge, I have a patches to clean up/enhance the > implementation and I''m planning to push to the qemu upstream soon. > > > >From 9a22525e523e834ab3822d14e715388c19a7d02e Mon Sep 17 00:00:00 > >2001 > Message-Id: > <9a22525e523e834ab3822d14e715388c19a7d02e.1276485897.git.yamahata@vali > nux.co.jp> > In-Reply-To: <cover.1276485897.git.yamahata@valinux.co.jp> > References: <cover.1276485897.git.yamahata@valinux.co.jp> > From: Isaku Yamahata <yamahata@valinux.co.jp> > Date: Mon, 14 Jun 2010 12:24:31 +0900 > Subject: [PATCH] pci passthrough: some clean up and unexport PCIBus and PCIBridge. > > some clean up and unexport PCIBus and PCIBridge. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/pass-through.h | 10 ++++++++-- > hw/pci.c | 29 +++++++++++++++++++++++------ > hw/pci.h | 23 ----------------------- > hw/piix_pci.c | 4 ++-- > hw/pt-graphics.c | 32 +++++++++++++++++++------------- > 5 files changed, 52 insertions(+), 46 deletions(-) > > diff --git a/hw/pass-through.h b/hw/pass-through.h index > 242d079..adc9f58 100644 > --- a/hw/pass-through.h > +++ b/hw/pass-through.h > @@ -409,13 +409,19 @@ uint8_t pci_intx(struct pt_dev *ptdev); > u32 pt_pci_host_read(int bus, int dev, int fn, u32 addr, int len); > int pt_pci_host_write(int bus, int dev, int fn, u32 addr, u32 val, int > len); void intel_pch_init(PCIBus *bus); -int igd_pci_write(PCIDevice > *pci_dev, int config_addr, uint32_t val, int len); -int > igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int > len); int register_vga_regions(struct pt_dev *real_device); int > unregister_vga_regions(struct pt_dev *real_device); int > setup_vga_pt(struct pt_dev *real_device); PCIBus > *intel_pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, > uint16_t did, const char *name, uint16_t revision); > > +#ifdef CONFIG_PASSTHROUGH > +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, > +int len); uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, > +int len); #else > +#define igd_pci_write(pci_dev, config_addr, val, len) NULL > +#define igd_pci_read(pci_dev, config_addr, val, len) NULL > +#endif > + > #endif /* __PASSTHROUGH_H__ */ > > diff --git a/hw/pci.c b/hw/pci.c > index a5cb378..b6400f3 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -39,6 +39,24 @@ extern int igd_passthru; > > //#define DEBUG_PCI > > +struct PCIBus { > + int bus_num; > + int devfn_min; > + pci_set_irq_fn set_irq; > + pci_map_irq_fn map_irq; > + uint32_t config_reg; /* XXX: suppress */ > + /* low level pic */ > + SetIRQFunc *low_set_irq; > + qemu_irq *irq_opaque; > + PCIDevice *devices[256]; > + PCIDevice *parent_dev; > + PCIBus *next; > + /* The bus IRQ state is the logical OR of the connected devices. > + Keep a count of the number of devices with raised IRQs. */ > + int nirq; > + int irq_count[]; > +}; > + > static void pci_update_mappings(PCIDevice *d); static void > pci_set_irq(void *opaque, int irq_num, int level); > > @@ -573,9 +591,6 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len) > pci_dev->name, config_addr, val, len); #endif > > -#ifdef CONFIG_PASSTHROUGH > - if (igd_pci_write(pci_dev, config_addr, val, len) == 0) > -#endif > pci_dev->config_write(pci_dev, config_addr, val, len); } > > @@ -610,9 +625,6 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len) > } > config_addr = addr & 0xff; > > -#ifdef CONFIG_PASSTHROUGH > - if ( igd_pci_read(pci_dev, config_addr, &val, len) == 0) > -#endif > val = pci_dev->config_read(pci_dev, config_addr, len); > > done_config_read: > @@ -860,6 +872,11 @@ void pci_unplug_netifs(void) > } > } > > +typedef struct { > + PCIDevice dev; > + PCIBus *bus; > +} PCIBridge; > + > void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { diff --git a/hw/pci.h b/hw/pci.h index 7a44035..8abbad7 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -210,29 +210,6 @@ struct PCIDevice { typedef void > (*pci_set_irq_fn)(qemu_irq *pic, int irq_num, int level); typedef int > (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > > -struct PCIBus { > - int bus_num; > - int devfn_min; > - pci_set_irq_fn set_irq; > - pci_map_irq_fn map_irq; > - uint32_t config_reg; /* XXX: suppress */ > - /* low level pic */ > - SetIRQFunc *low_set_irq; > - qemu_irq *irq_opaque; > - PCIDevice *devices[256]; > - PCIDevice *parent_dev; > - PCIBus *next; > - /* The bus IRQ state is the logical OR of the connected devices. > - Keep a count of the number of devices with raised IRQs. */ > - int nirq; > - int irq_count[]; > -}; > - > -typedef struct { > - PCIDevice dev; > - PCIBus *bus; > -} PCIBridge; > - > extern char direct_pci_str[]; > extern int direct_pci_msitranslate; > extern int direct_pci_power_mgmt; > diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 49d72b2..1bfe0fb > 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -25,7 +25,7 @@ > #include "hw.h" > #include "pc.h" > #include "pci.h" > - > +#include "pass-through.h" > > static void i440fx_set_irq(qemu_irq *pic, int irq_num, int level); > static void piix3_write_config(PCIDevice *d, @@ -207,7 +207,7 @@ > PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic) > register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s); > > d = pci_register_device(b, "i440FX", sizeof(PCIDevice), 0, > - NULL, NULL); > + igd_pci_read, igd_pci_write); > > pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_INTEL); > pci_config_set_device_id(d->config, PCI_DEVICE_ID_INTEL_82441); > diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c index > 4923715..32c174d 100644 > --- a/hw/pt-graphics.c > +++ b/hw/pt-graphics.c > @@ -8,6 +8,7 @@ > > #include <unistd.h> > #include <sys/ioctl.h> > +#include <assert.h> > > extern int gfx_passthru; > extern int igd_passthru; > @@ -34,10 +35,13 @@ void intel_pch_init(PCIBus *bus) > pch_map_irq, "intel_bridge_1f"); } > > -int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, > int len) > +void igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, > +int len) > { > - if ( !igd_passthru || (pci_dev->devfn != 0x00 ) ) > - return 0; > + assert(pci_dev->devfn == 0x00); > + if ( !igd_passthru ) { > + pci_default_write_config(pci_dev, config_addr, val, len); > + return; > + } > > switch (config_addr) > { > @@ -48,15 +52,18 @@ int igd_pci_write(PCIDevice *pci_dev, int config_addr, uint32_t val, int len) > PCI_FUNC(pci_dev->devfn), config_addr, len, val); > break; > default: > - pci_dev->config_write(pci_dev, config_addr, val, len); > + pci_default_write_config(pci_dev, config_addr, val, len); > } > - return 1; > } > > -int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, > int len) > +uint32_t igd_pci_read(PCIDevice *pci_dev, int config_addr, int len) > { > - if ( !igd_passthru || (pci_dev->devfn != 0) ) > - return 0; > + uint32_t val; > + > + assert(pci_dev->devfn == 0x00); > + if ( !igd_passthru ) { > + return pci_default_read_config(pci_dev, config_addr, len); > + } > > switch (config_addr) > { > @@ -68,17 +75,16 @@ int igd_pci_read(PCIDevice *pci_dev, int config_addr, uint32_t *val, int len) > case 0x58: /* SNB: PAVPC Offset */ > case 0xa4: /* SNB: graphics base of stolen memory */ > case 0xa8: /* SNB: base of GTT stolen memory */ > - *val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), > + val = pt_pci_host_read(0, PCI_SLOT(pci_dev->devfn), > 0, config_addr, len); > PT_LOG("pci_config_read: %x:%x.%x: addr=%x len=%x val=%x\n", > pci_bus_num(pci_dev->bus), PCI_SLOT(pci_dev->devfn), > - PCI_FUNC(pci_dev->devfn), config_addr, len, *val); > - > + PCI_FUNC(pci_dev->devfn), config_addr, len, val); > break; > default: > - *val = pci_dev->config_read(pci_dev, config_addr, len); > + val = pci_default_read_config(pci_dev, config_addr, len); > } > - return 1; > + return val; > } > > /* > -- > 1.6.6.1 > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >-- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-18 11:40 UTC
RE: [Xen-devel] [PATCH][VTD] enable integrated graphics passthrough for Calpella and Sandybridge
On Fri, 18 Jun 2010, Kay, Allen M wrote:> Isaku/Stafano, > > Just want to report that this patch worked without any problems. As long as everyone is OK with hooking igd_pci_read/write functions in piix_pci.c, I''m fine with it too. I wasn''t sure about it earlier ... >It is OK by me. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel