WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH V3 07/10] Introduce Xen PCI Passthrough, qdevice

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