Re: [Qemu-devel] [Xen-devel] [PATCH V3 07/10] Introduce Xen PCI Passthrough, qdevice (1/3)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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