> > > + 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
|
Home