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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 18 May 2018 14:34
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; qemu-devel@nongnu.org;
> ehabkost@redhat.com; marcel@redhat.com; mst@redhat.com; Paolo
> Bonzini <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>
> Subject: Re: [Xen-devel] [PATCH v2] xen-hvm: stop faking I/O to access PCI
> config space
> 
> >>> On 18.05.18 at 15:00, <paul.durrant@citrix.com> wrote:
> > @@ -903,6 +926,80 @@ static void cpu_ioreq_move(ioreq_t *req)
> >      }
> >  }
> >
> > +static void rw_config_req_item(XenPciDevice *xendev, ioreq_t *req,
> 
> It looks to me as if both parameters could be constified.
> 

They could for this function, yes.

> > +                               uint32_t i, uint32_t *val)
> > +{
> > +    int32_t reg = req->addr;
> > +    uint32_t offset = req->size * i;
> > +
> > +    reg += (req->df ? -1 : 1) * offset;
> > +    if (reg < 0 || reg > PCI_CONFIG_SPACE_SIZE) {
> 
> Having fought a number of issues in this area in the hypervisor a couple
> of years back I wonder
> - why reg is of signed type,

I did that so I could do a < 0 check.

> - whether overflow of the first multiplication really doesn't matter,

It would be better to check it.

> - whether wrapping when adding in the offset is not an issue.
> 

I'll do limits check on offset then... should be able to make reg unsigned then I guess.

> I take it that the rather lax upper bound check (should imo really be
> reg + size > PCI_CONFIG_SPACE_SIZE [implying reg + size doesn't
> itself wrap], or at least reg >= PCI_CONFIG_SPACE_SIZE) is not a
> problem because ...
> 
> > +        if (req->dir == IOREQ_READ) {
> > +            *val = ~0u;
> > +        }
> > +        return;
> > +    }
> > +
> > +    if (req->dir == IOREQ_READ) {
> > +        *val = pci_host_config_read_common(xendev->pci_dev, reg,
> > +                                           PCI_CONFIG_SPACE_SIZE,
> > +                                           req->size);
> > +        trace_cpu_ioreq_config_read(req, xendev->sbdf, reg,
> > +                                    req->size, *val);
> > +    } else {
> > +        trace_cpu_ioreq_config_write(req, xendev->sbdf, reg, req->size,
> > +                                     *val);
> > +        pci_host_config_write_common(xendev->pci_dev, reg,
> > +                                     PCI_CONFIG_SPACE_SIZE, *val,
> > +                                     req->size);
> > +    }
> 
> ... these called functions do full checking anyway?

Yes, I'm deferring further checking to these common functions. I'm only intending to avoid passing junk into them here.

> 
> > +static void cpu_ioreq_config(XenIOState *state, ioreq_t *req)
> > +{
> > +    uint32_t sbdf = req->addr >> 32;
> > +    XenPciDevice *xendev;
> > +
> > +    if (req->size > sizeof(uint32_t)) {
> > +        hw_error("PCI config access: bad size (%u)", req->size);
> 
> What about size 0 or 3?
> 

Yes, I can reject those here also.

> > +    }
> > +
> > +    QLIST_FOREACH(xendev, &state->dev_list, entry) {
> > +        unsigned int i;
> > +        uint32_t tmp;
> > +
> > +        if (xendev->sbdf != sbdf) {
> > +            continue;
> > +        }
> > +
> > +        if (!req->data_is_ptr) {
> > +            if (req->dir == IOREQ_READ) {
> > +                for (i = 0; i < req->count; i++) {
> > +                    rw_config_req_item(xendev, req, i, &tmp);
> > +                    req->data = tmp;
> > +                }
> > +            } else if (req->dir == IOREQ_WRITE) {
> > +                for (i = 0; i < req->count; i++) {
> > +                    tmp = req->data;
> > +                    rw_config_req_item(xendev, req, i, &tmp);
> > +                }
> > +            }
> 
> Wouldn't it be more sensible to fail req->count != 1 requests here?
> 

I'm wondering whether we'd want to handle count > 1 once we allow MMCONFIG accesses though. I guess it would be easier just to defer that.

  Paul

> Jan
>