[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V3 07/10] Introduce Xen PCI Passthrough, qdevice (1/3)
> > > + hw_error("Internal error: Invalid write emulation " > > > + "return value[%d]. I/O emulator exit.\n", rc); > > > > Oh. I hadn't realized this, but you are using hw_error. Which is > > calling 'abort'! Yikes. Is there no way to recover from this? Say return > > 0xfffff? > > In qemu-xen-traditionnal, it was an exit(1). I do not know the > consequence of a bad write, and I can not return anythings. So I suppose > that the guest would know that somethings wrong only on the next read. > > Instead of abort();, I can just do nothing and return. Or we could unplug > the device from QEMU. > > Any preference? I think this calls for an experiment. If Linux still functions if you completly unplug the device, then I would say unplug it (b/c in most likelyhood the reason you can't write is b/c the host has unplugged the device). > > > > + } > > > + > > > + /* calculate next address to find */ > > > + emul_len -= reg->size; > > > + if (emul_len > 0) { > > > + find_addr = real_offset + reg->size; > > > + } > > > + } else { > > > + /* nothing to do with passthrough type register, > > > + * continue to find next byte */ > > > + emul_len--; > > > + find_addr++; > > > + } > > > + } > > > + > > > + /* need to shift back before passing them to libpci */ > > > + val >>= (address & 3) << 3; > > > + > > > +out: > > > + if (!(reg && reg->no_wb)) { > > > + /* unknown regs are passed through */ > > > + rc = host_pci_set_block(s->real_device, address, (uint8_t > > > *)&val, len); > > > + > > > + if (!rc) { > > > + PT_LOG("Error: pci_write_block failed. return value[%d].\n", > > > rc); > > > + } > > > + } > > > + > > > + if (s->pm_state != NULL && s->pm_state->flags & PT_FLAG_TRANSITING) { > > > + qemu_mod_timer(s->pm_state->pm_timer, > > > + qemu_get_clock_ms(rt_clock) + > > > s->pm_state->pm_delay); > > > + } > > > +} > > > + > > > +/* ioport/iomem space*/ > > > +static void pt_iomem_map(XenPCIPassthroughState *s, int i, > > > + pcibus_t e_phys, pcibus_t e_size, int type) > > > +{ > > > + uint32_t old_ebase = s->bases[i].e_physbase; > > > + bool first_map = s->bases[i].e_size == 0; > > > + int ret = 0; > > > + > > > + s->bases[i].e_physbase = e_phys; > > > + s->bases[i].e_size = e_size; > > > + > > > + PT_LOG("e_phys=%#"PRIx64" maddr=%#"PRIx64" type=%%d" > > > + " len=%#"PRIx64" index=%d first_map=%d\n", > > > + e_phys, s->bases[i].access.maddr, /*type,*/ > > > + e_size, i, first_map); > > > + > > > + if (e_size == 0) { > > > + return; > > > + } > > > + > > > + if (!first_map && old_ebase != -1) { > > > > old_ebase != PCI_BAR_UNMAPPED ? > > :(, no. Because old_ebase is a uint32_t and PCI_BAR_UNMAPPED is > pcibus_t (uint64_t in Xen case). I somehow thought it was defined as -1.. but > > I'm not sure that a good idee to change the type of old_ebase as > xc_domain_memory_mapping bellow takes only uint32_t. > > But, if I can replace a -1 by PCI_BAR_UNMAPPED, I will. .. or something close to it. _PCI_BAR_UNMAPPED? .. snip.. > > > + /* Register PIO/MMIO BARs */ > > > + for (i = 0; i < PCI_BAR_ENTRIES; i++) { > > > + HostPCIIORegion *r = &d->io_regions[i]; > > > + > > > + if (r->base_addr) { > > > > So should you check for PCI_BAR_UNMAPPED or is that not really > > required here as the pci_register_bar would do it? > > Actually, this value come from the real device (the value in > sysfs/resource). So, I think it's just 0 if it's not mapped. Ah! Right. > > Here, it's probably better to check for the size instead, to know if > there is actually a BAR. <nods> > > > > + s->bases[i].e_physbase = r->base_addr; > > > + s->bases[i].access.u = r->base_addr; > > > + > > > + /* Register current region */ > > > + if (r->flags & IORESOURCE_IO) { > > > + memory_region_init_io(&s->bar[i], NULL, NULL, > > > + "xen-pci-pt-bar", r->size); > > > > You can make the "xen_pci-pt-bar" be a #define somewhere and reuse that. .. snip .. > > > + if (!s->dev.config[PCI_INTERRUPT_PIN]) { > > > + PT_LOG("no pin interrupt\n"); > > > > Perhaps include some details of which device failed? > > There is already detailed about the device at the beginning of the > function. Is it not enough? I was thinking parallel operations. So it could be there are multiple PCI requests and you might not know which device's pin is wrong. > > > > + goto out; > > > + } > > > + > > > + machine_irq = host_pci_get_byte(s->real_device, PCI_INTERRUPT_LINE); > > > + rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); > > > + > > > + if (rc) { > > > + PT_LOG("Error: Mapping irq failed, rc = %d\n", rc); > > > > Can you also include the IRQ it tried to map (both machine and pirq). > > Yep. > > > > + > > > + /* Disable PCI intx assertion (turn on bit10 of devctl) */ > > > + host_pci_set_word(s->real_device, > > > + PCI_COMMAND, > > > + pci_get_word(s->dev.config + PCI_COMMAND) > > > + | PCI_COMMAND_INTX_DISABLE); > > > + machine_irq = 0; > > > + s->machine_irq = 0; > > > + } else { > > > + machine_irq = pirq; > > > + s->machine_irq = pirq; > > > + mapped_machine_irq[machine_irq]++; > > > + } > > > + > > > + /* bind machine_irq to device */ > > > + if (rc < 0 && machine_irq != 0) { > > > + uint8_t e_device = PCI_SLOT(s->dev.devfn); > > > + uint8_t e_intx = pci_intx(s); > > > + > > > + rc = xc_domain_bind_pt_pci_irq(xen_xc, xen_domid, machine_irq, 0, > > > + e_device, e_intx); > > > + if (rc < 0) { > > > + PT_LOG("Error: Binding of interrupt failed! rc=%d\n", rc); > > > > A bit details - name of the device, the IRQ,.. > > > > > + > > > + /* Disable PCI intx assertion (turn on bit10 of devctl) */ > > > + host_pci_set_word(s->real_device, PCI_COMMAND, > > > + *(uint16_t *)(&s->dev.config[PCI_COMMAND]) > > > + | PCI_COMMAND_INTX_DISABLE); > > > + mapped_machine_irq[machine_irq]--; > > > + > > > + if (mapped_machine_irq[machine_irq] == 0) { > > > + if (xc_physdev_unmap_pirq(xen_xc, xen_domid, > > > machine_irq)) { > > > + PT_LOG("Error: Unmapping of interrupt failed! > > > rc=%d\n", > > > + rc); > > > > And here too. It would be beneficial to have on the error paths lots of > > nice details so that in the field it will be easier to find out what > > went wrong (and match up PIRQ with the GSI). > > Yes, I will try to improve the messages. > > It's also probably good to always print the errors. <nods> Thanks. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |