From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54094) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROvUa-0007yA-CE for qemu-devel@nongnu.org; Fri, 11 Nov 2011 13:06:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ROvUY-0005V7-Ku for qemu-devel@nongnu.org; Fri, 11 Nov 2011 13:06:20 -0500 Received: from acsinet15.oracle.com ([141.146.126.227]:44001) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROvUY-0005Ul-EN for qemu-devel@nongnu.org; Fri, 11 Nov 2011 13:06:18 -0500 Date: Fri, 11 Nov 2011 13:05:42 -0500 From: Konrad Rzeszutek Wilk Message-ID: <20111111180542.GA6408@phenom.dumpdata.com> References: <1319814456-8158-1-git-send-email-anthony.perard@citrix.com> <1319814456-8158-8-git-send-email-anthony.perard@citrix.com> <20111110212840.GA23643@phenom.dumpdata.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [Xen-devel] [PATCH V3 07/10] Introduce Xen PCI Passthrough, qdevice (1/3) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony PERARD Cc: Guy Zana , Xen Devel , Allen Kay , QEMU-devel , Stefano Stabellini > > > + 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. > > > > + 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. Thanks.