Re: [Qemu-devel] [PATCH] xen-hvm: stop faking I/O to access PCI config space

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 17 May 2018 17:31
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; Stefano
> Stabellini <sstabellini@kernel.org>; Michael S. Tsirkin <mst@redhat.com>;
> Marcel Apfelbaum <marcel@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo
> Habkost <ehabkost@redhat.com>
> Subject: Re: [PATCH] xen-hvm: stop faking I/O to access PCI config space
> 
> On Thu, May 03, 2018 at 12:18:40PM +0100, Paul Durrant wrote:
> > This patch removes the current hackery where IOREQ_TYPE_PCI_CONFIG
> > reqyests are handled by faking PIO to 0xcf8 and 0xcfc and replaces it
> 
>   ^ requests

Ok.

> 
> > with direct calls to pci_host_config_read/write_common().
> > Doing so necessitates mapping BDFs to PCIDevices but maintaining a simple
> > QLIST in xen_device_realize/unrealize() will suffice.
> >
> > NOTE: whilst config space accesses are currently limited to
> >       PCI_CONFIG_SPACE_SIZE, this patch paves the way to increasing the
> >       limit to PCIE_CONFIG_SPACE_SIZE when Xen gains the ability to
> >       emulate MCFG table accesses.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> > +static void cpu_ioreq_config(XenIOState *state, ioreq_t *req)
> > +{
> > +    uint32_t sbdf = req->addr >> 32;
> > +    uint32_t reg = req->addr;
> > +    XenPciDevice *xendev;
> > +
> > +    if (req->size > sizeof(uint32_t)) {
> > +        hw_error("PCI config access: bad size (%u)", req->size);
> > +    }
> > +
> > +    QLIST_FOREACH(xendev, &state->dev_list, entry) {
> > +        unsigned int i;
> > +
> > +        if (xendev->sbdf != sbdf) {
> > +            continue;
> > +        }
> > +
> > +        if (req->dir == IOREQ_READ) {
> > +            if (!req->data_is_ptr) {
> > +                req->data = pci_host_config_read_common(
> > +                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
> > +                    req->size);
> > +                trace_cpu_ioreq_config_read(req, sbdf, reg, req->size,
> > +                                            req->data);
> > +            } else {
> > +                for (i = 0; i < req->count; i++) {
> > +                    uint32_t tmp;
> > +
> > +                    tmp = pci_host_config_read_common(
> > +                        xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE,
> > +                        req->size);
> 
> So, if data is a pointer, we just keep reading the same address
> req->count time?
> 

That's what would have happened before AFAICT, since the old scheme used port I/O. I think you're right that it is probably worth changing to MMIO semantics with this change though.

  Paul

> > +                    write_phys_req_item(req->data, req, i, &tmp);
> > +                }
> > +            }
> > +        } else if (req->dir == IOREQ_WRITE) {
> > +            if (!req->data_is_ptr) {
> > +                trace_cpu_ioreq_config_write(req, sbdf, reg, req->size,
> > +                                             req->data);
> > +                pci_host_config_write_common(
> > +                    xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE, req->data,
> > +                    req->size);
> > +            } else {
> > +                for (i = 0; i < req->count; i++) {
> > +                    uint32_t tmp = 0;
> > +
> > +                    read_phys_req_item(req->data, req, i, &tmp);
> > +                    pci_host_config_write_common(
> > +                        xendev->pci_dev, reg, PCI_CONFIG_SPACE_SIZE, tmp,
> > +                        req->size);
> > +                }
> > +            }
> > +        }
> > +    }
> > +}
> 
> --
> Anthony PERARD