Re: [PATCH RESEND v6 21/36] multi-process: PCI BAR read/write handling for proxy & remote endpoints

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:56PM -0700, address@hidden wrote:
> +uint64_t proxy_default_bar_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    ProxyMemoryRegion *pmr = opaque;
> +    uint64_t val;
> +
> +    send_bar_access_msg(pmr->dev, &pmr->mr, false, addr, &val, size,
> +                        pmr->memory);
> +
> +     return val;

Indentation is should be 4 spaces.

> @@ -43,4 +54,9 @@ typedef struct PCIProxyDevClass {
>      char *command;
>  } PCIProxyDevClass;
>  
> +void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val,
> +                             unsigned size);
> +
> +uint64_t proxy_default_bar_read(void *opaque, hwaddr addr, unsigned size);

"default" is probably not appropriate here. This isn't a base class'
default implementation that child classes override.

> @@ -114,6 +115,77 @@ exit:
>      notify_proxy(wait, ret);
>  }
>  
> +/* TODO: confirm memtx attrs. */
> +static void process_bar_write(MPQemuMsg *msg, Error **errp)
> +{
> +    bar_access_msg_t *bar_access = &msg->data1.bar_access;
> +    AddressSpace *as =
> +        bar_access->memory ? &address_space_memory : &address_space_io;
> +    MemTxResult res;
> +
> +    assert(is_power_of_2(bar_access->size) &&
> +           bar_access->size <= sizeof(uint64_t));

Please return an error instead of asserting. At the moment it doesn't
matter much but this will be necessary so that a process hosting remote
devices for multiple guests cannot be killed by one malicious guest.

> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (uint8_t *)&bar_access->val, bar_access->size,

Missing uint64_t val to void* conversion code.

For example, if bar_access->size is 1 then this fails on big-endian
hosts because &bar_access->val is the address of the most-significant
byte in uint64_t val:

  uint64_t val = 1;

Now we have the following memory layout on big-endian hosts:

  val = 00 00 00 00  00 00 00 01
        ^-------------- &val

We actually wanted a 1-byte write of 0x01 and not 0x00.

signature.asc
Description: PGP signature