[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] [PATCH v2] xen-hvm: stop faking I/O to acce
From: |
Jan Beulich |
Subject: |
Re: [Qemu-devel] [Xen-devel] [PATCH v2] xen-hvm: stop faking I/O to access PCI config space |
Date: |
Fri, 18 May 2018 07:33:53 -0600 |
>>> On 18.05.18 at 15:00, <address@hidden> 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.
> + 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,
- whether overflow of the first multiplication really doesn't matter,
- whether wrapping when adding in the offset is not an issue.
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?
> +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?
> + }
> +
> + 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?
Jan