Re: [PATCH v8 13/20] multi-process: PCI BAR read/write handling for proxy & remote endpoints

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4778 bytes --]

On Fri, Jul 31, 2020 at 02:20:20PM -0400, Jagannathan Raman wrote:
> +static void process_bar_write(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
> +{
> +    BarAccessMsg *bar_access = &msg->data1.bar_access;
> +    AddressSpace *as =
> +        bar_access->memory ? &address_space_memory : &address_space_io;
> +    MPQemuMsg ret = { 0 };
> +    MemTxResult res;
> +    uint64_t val;
> +    Error *local_err = NULL;
> +
> +    if (!is_power_of_2(bar_access->size) ||
> +       (bar_access->size > sizeof(uint64_t))) {
> +        ret.data1.u64 = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +    val = cpu_to_le64(bar_access->val);
> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (void *)&val, bar_access->size, true);
> +
> +    if (res != MEMTX_OK) {
> +        error_setg(errp, "Could not perform address space write operation,"
> +                   " inaccessible address: %lx in pid %d.",
> +                   bar_access->addr, getpid());
> +        ret.data1.u64 = -1;
> +    }
> +
> +fail:
> +    ret.cmd = RET_MSG;
> +    ret.size = sizeof(ret.data1);
> +
> +    mpqemu_msg_send(&ret, ioc, &local_err);
> +    if (local_err) {
> +        error_setg(errp, "Error while sending message to proxy "
> +                   "in remote process pid=%d", getpid());

There is an assertion failure if res != MEMTX_OK because errp was
already set. error_setg() must not be called on an Error pointer that
has already been set.

It is simplest to do:

  mpqemu_msg_send(&ret, ioc, (errp && *errp) ? NULL : &local_err);

> +    }
> +}
> +
> +static void process_bar_read(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
> +{
> +    BarAccessMsg *bar_access = &msg->data1.bar_access;
> +    MPQemuMsg ret = { 0 };
> +    AddressSpace *as;
> +    MemTxResult res;
> +    uint64_t val = 0;
> +    Error *local_err = NULL;
> +
> +    as = bar_access->memory ? &address_space_memory : &address_space_io;
> +
> +    if (!is_power_of_2(bar_access->size) ||
> +       (bar_access->size > sizeof(uint64_t))) {
> +        val = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (void *)&val, bar_access->size, false);
> +
> +    if (res != MEMTX_OK) {
> +        error_setg(errp, "Could not perform address space read operation,"
> +                   " inaccessible address: %lx in pid %d.",
> +                   bar_access->addr, getpid());
> +        val = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +fail:
> +    ret.cmd = RET_MSG;
> +    ret.data1.u64 = le64_to_cpu(val);
> +    ret.size = sizeof(ret.data1);
> +
> +    mpqemu_msg_send(&ret, ioc, &local_err);
> +    if (local_err) {
> +        error_setg(errp, "Error while sending message to proxy "

Same here.

> +static void send_bar_access_msg(PCIProxyDev *pdev, MemoryRegion *mr,
> +                                bool write, hwaddr addr, uint64_t *val,
> +                                unsigned size, bool memory)
> +{
> +    MPQemuMsg msg = { 0 };
> +    long ret = -EINVAL;

long is not guaranteed to be 64-bit. This function supports 64-bit
accesses to BARs so uint64_t is needed here.

> +    Error *local_err = NULL;
> +
> +    msg.bytestream = 0;
> +    msg.size = sizeof(msg.data1);
> +    msg.data1.bar_access.addr = mr->addr + addr;
> +    msg.data1.bar_access.size = size;
> +    msg.data1.bar_access.memory = memory;
> +
> +    if (write) {
> +        msg.cmd = BAR_WRITE;
> +        msg.data1.bar_access.val = *val;
> +    } else {
> +        msg.cmd = BAR_READ;
> +    }
> +
> +    ret = mpqemu_msg_send_and_await_reply(&msg, pdev->ioc, &local_err);
> +    if (local_err) {
> +        error_report("Failed to send BAR command to the remote process.");

Leaks local_err. Please report the error message from local_err and then
free it.

> +const MemoryRegionOps proxy_mr_ops = {
> +    .read = proxy_bar_read,
> +    .write = proxy_bar_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,

Should this be .max_access_size = 8?

> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index 5d04b81..82b8465 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -269,6 +269,12 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
>              return false;
>          }
>          break;
> +    case BAR_WRITE:
> +    case BAR_READ:
> +        if ((msg->size != sizeof(msg->data1)) || (msg->num_fds != 0)) {

What about bytestream? It would be cleanest not to send bytestream over
the wire. Since it is sent today the receiver can be confused if it has
the wrong value and some mpqemu_msg_valid() cases validate bytestream.
It is not validate here for some reason.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]