[v5,15/50] multi-process: PCI BAR read/write handling for proxy & remote endpoints

Proxy device object implements handler for PCI BAR writes and reads. The handler
uses BAR_WRITE/BAR_READ message to communicate to the remote process with the BAR address and
value to be written/read.
The remote process implements handler for BAR_WRITE/BAR_READ message.

Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
---
 hw/proxy/qemu-proxy.c         | 65 ++++++++++++++++++++++++++++++++++++++
 include/hw/proxy/qemu-proxy.h | 22 +++++++++++--
 include/io/mpqemu-link.h      | 12 +++++++
 remote/remote-main.c          | 73 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+), 2 deletions(-)

* Jagannathan Raman (jag.raman@oracle.com) wrote:
> Proxy device object implements handler for PCI BAR writes and reads. The handler
> uses BAR_WRITE/BAR_READ message to communicate to the remote process with the BAR address and
> value to be written/read.
> The remote process implements handler for BAR_WRITE/BAR_READ message.
> 
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> ---
>  hw/proxy/qemu-proxy.c         | 65 ++++++++++++++++++++++++++++++++++++++
>  include/hw/proxy/qemu-proxy.h | 22 +++++++++++--
>  include/io/mpqemu-link.h      | 12 +++++++
>  remote/remote-main.c          | 73 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 170 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
> index d792e86..b17d9bb 100644
> --- a/hw/proxy/qemu-proxy.c
> +++ b/hw/proxy/qemu-proxy.c
> @@ -262,3 +262,68 @@ static void pci_proxy_dev_realize(PCIDevice *device, Error **errp)
>      dev->get_proxy_sock = get_proxy_sock;
>      dev->init_proxy = init_proxy;
>  }
> +
> +static void send_bar_access_msg(PCIProxyDev *dev, MemoryRegion *mr,
> +                                bool write, hwaddr addr, uint64_t *val,
> +                                unsigned size, bool memory)
> +{
> +    MPQemuLinkState *mpqemu_link = dev->mpqemu_link;
> +    MPQemuMsg msg;
> +    int wait;
> +
> +    memset(&msg, 0, sizeof(MPQemuMsg));
> +
> +    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 {
> +        wait = GET_REMOTE_WAIT;
> +
> +        msg.cmd = BAR_READ;
> +        msg.num_fds = 1;
> +        msg.fds[0] = wait;
> +    }
> +
> +    mpqemu_msg_send(&msg, mpqemu_link->com);
> +
> +    if (!write) {
> +        *val = wait_for_remote(wait);
> +        PUT_REMOTE_WAIT(wait);
> +    }
> +}
> +
> +void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val,
> +                             unsigned size)
> +{
> +    ProxyMemoryRegion *pmr = opaque;
> +
> +    send_bar_access_msg(pmr->dev, &pmr->mr, true, addr, &val, size,
> +                        pmr->memory);
> +}
> +
> +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;
> +}
> +
> +const MemoryRegionOps proxy_default_ops = {
> +    .read = proxy_default_bar_read,
> +    .write = proxy_default_bar_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> diff --git a/include/hw/proxy/qemu-proxy.h b/include/hw/proxy/qemu-proxy.h
> index 29fa2e9..44e370e 100644
> --- a/include/hw/proxy/qemu-proxy.h
> +++ b/include/hw/proxy/qemu-proxy.h
> @@ -22,7 +22,19 @@
>  #define PCI_PROXY_DEV_GET_CLASS(obj) \
>              OBJECT_GET_CLASS(PCIProxyDevClass, (obj), TYPE_PCI_PROXY_DEV)
>  
> -typedef struct PCIProxyDev {
> +typedef struct PCIProxyDev PCIProxyDev;
> +
> +typedef struct ProxyMemoryRegion {
> +    PCIProxyDev *dev;
> +    MemoryRegion mr;
> +    bool memory;
> +    bool present;
> +    uint8_t type;
> +} ProxyMemoryRegion;
> +
> +extern const MemoryRegionOps proxy_default_ops;
> +
> +struct PCIProxyDev {
>      PCIDevice parent_dev;
>  
>      MPQemuLinkState *mpqemu_link;
> @@ -41,7 +53,8 @@ typedef struct PCIProxyDev {
>      void (*init_proxy) (PCIDevice *dev, char *command, char *exec_name,
>                          bool need_spawn, Error **errp);
>  
> -} PCIProxyDev;
> +    ProxyMemoryRegion region[PCI_NUM_REGIONS];
> +};
>  
>  typedef struct PCIProxyDevClass {
>      PCIDeviceClass parent_class;
> @@ -51,4 +64,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);
> +
>  #endif /* QEMU_PROXY_H */
> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> index 5a2be48..1a7738e 100644
> --- a/include/io/mpqemu-link.h
> +++ b/include/io/mpqemu-link.h
> @@ -38,6 +38,8 @@
>   * PCI_CONFIG_READ        PCI configuration space read
>   * PCI_CONFIG_WRITE       PCI configuration space write
>   * SYNC_SYSMEM      Shares QEMU's RAM with remote device's RAM
> + * BAR_WRITE        Writes to PCI BAR region
> + * BAR_READ         Reads from PCI BAR region
>   *
>   * proc_cmd_t enum type to specify the command to be executed on the remote
>   * device.
> @@ -47,6 +49,8 @@ typedef enum {
>      PCI_CONFIG_READ,
>      PCI_CONFIG_WRITE,
>      SYNC_SYSMEM,
> +    BAR_WRITE,
> +    BAR_READ,
>      MAX,
>  } mpqemu_cmd_t;
>  
> @@ -70,6 +74,13 @@ typedef struct {
>  } sync_sysmem_msg_t;
>  
>  typedef struct {
> +    hwaddr addr;
> +    uint64_t val;
> +    unsigned size;
> +    bool memory;
> +} bar_access_msg_t;
> +
> +typedef struct {
>      mpqemu_cmd_t cmd;
>      int bytestream;
>      size_t size;
> @@ -77,6 +88,7 @@ typedef struct {
>      union {
>          uint64_t u64;
>          sync_sysmem_msg_t sync_sysmem;
> +        bar_access_msg_t bar_access;
>      } data1;
>  
>      int fds[REMOTE_MAX_FDS];
> diff --git a/remote/remote-main.c b/remote/remote-main.c
> index 7b4cf2f..acd8daf 100644
> --- a/remote/remote-main.c
> +++ b/remote/remote-main.c
> @@ -33,6 +33,7 @@
>  #include "sysemu/sysemu.h"
>  #include "block/block.h"
>  #include "exec/ramlist.h"
> +#include "exec/memattrs.h"
>  
>  static MPQemuLinkState *mpqemu_link;
>  PCIDevice *remote_pci_dev;
> @@ -63,6 +64,66 @@ static void process_config_read(MPQemuMsg *msg)
>      PUT_REMOTE_WAIT(wait);
>  }
>  
> +/* 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;
> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (uint8_t *)&bar_access->val, bar_access->size, true);
> +
> +    if (res != MEMTX_OK) {
> +        error_setg(errp, "Could not perform address space write operation,"
> +                   " inaccessible address: %lx.", bar_access->addr);
> +    }
> +}
> +
> +static void process_bar_read(MPQemuMsg *msg, Error **errp)
> +{
> +    bar_access_msg_t *bar_access = &msg->data1.bar_access;
> +    AddressSpace *as;
> +    int wait = msg->fds[0];
> +    MemTxResult res;
> +    uint64_t val = 0;
> +
> +    as = bar_access->memory ? &address_space_memory : &address_space_io;
> +
> +    assert(bar_access->size <= sizeof(uint64_t));

Note you don't have that check on the write function above.
Do you actually want something like:
   assert(is_power_of_2(bar_access->size) && bar_access->size <= sizeof(uint64_t));

> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (uint8_t *)&val, bar_access->size, false);
> +
> +    if (res != MEMTX_OK) {
> +        error_setg(errp, "Could not perform address space read operation,"
> +                   " inaccessible address: %lx.", bar_access->addr);
> +        val = (uint64_t)-1;
> +        goto fail;
> +    }
> +
> +    switch (bar_access->size) {

No case 8 ?

Dave

> +    case 4:
> +        val = *((uint32_t *)&val);
> +        break;
> +    case 2:
> +        val = *((uint16_t *)&val);
> +        break;
> +    case 1:
> +        val = *((uint8_t *)&val);
> +        break;
> +    default:
> +        error_setg(errp, "Invalid PCI BAR read size");
> +        return;
> +    }
> +
> +fail:
> +    notify_proxy(wait, val);
> +
> +    PUT_REMOTE_WAIT(wait);
> +}
> +
>  static void process_msg(GIOCondition cond, MPQemuChannel *chan)
>  {
>      MPQemuMsg *msg = NULL;
> @@ -88,6 +149,18 @@ static void process_msg(GIOCondition cond, MPQemuChannel *chan)
>      case PCI_CONFIG_READ:
>          process_config_read(msg);
>          break;
> +    case BAR_WRITE:
> +        process_bar_write(msg, &err);
> +        if (err) {
> +            goto finalize_loop;
> +        }
> +        break;
> +    case BAR_READ:
> +        process_bar_read(msg, &err);
> +        if (err) {
> +            goto finalize_loop;
> +        }
> +        break;
>      default:
>          error_setg(&err, "Unknown command");
>          goto finalize_loop;
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 3/4/2020 5:47 AM, Dr. David Alan Gilbert wrote:
> * Jagannathan Raman (jag.raman@oracle.com) wrote:
>> Proxy device object implements handler for PCI BAR writes and reads. The handler
>> uses BAR_WRITE/BAR_READ message to communicate to the remote process with the BAR address and
>> value to be written/read.
>> The remote process implements handler for BAR_WRITE/BAR_READ message.
>>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> ---
>>   hw/proxy/qemu-proxy.c         | 65 ++++++++++++++++++++++++++++++++++++++
>>   include/hw/proxy/qemu-proxy.h | 22 +++++++++++--
>>   include/io/mpqemu-link.h      | 12 +++++++
>>   remote/remote-main.c          | 73 +++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 170 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
>> index d792e86..b17d9bb 100644
>> --- a/hw/proxy/qemu-proxy.c
>> +++ b/hw/proxy/qemu-proxy.c
>> @@ -262,3 +262,68 @@ static void pci_proxy_dev_realize(PCIDevice *device, Error **errp)
>>       dev->get_proxy_sock = get_proxy_sock;
>>       dev->init_proxy = init_proxy;
>>   }
>> +
>> +static void send_bar_access_msg(PCIProxyDev *dev, MemoryRegion *mr,
>> +                                bool write, hwaddr addr, uint64_t *val,
>> +                                unsigned size, bool memory)
>> +{
>> +    MPQemuLinkState *mpqemu_link = dev->mpqemu_link;
>> +    MPQemuMsg msg;
>> +    int wait;
>> +
>> +    memset(&msg, 0, sizeof(MPQemuMsg));
>> +
>> +    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 {
>> +        wait = GET_REMOTE_WAIT;
>> +
>> +        msg.cmd = BAR_READ;
>> +        msg.num_fds = 1;
>> +        msg.fds[0] = wait;
>> +    }
>> +
>> +    mpqemu_msg_send(&msg, mpqemu_link->com);
>> +
>> +    if (!write) {
>> +        *val = wait_for_remote(wait);
>> +        PUT_REMOTE_WAIT(wait);
>> +    }
>> +}
>> +
>> +void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val,
>> +                             unsigned size)
>> +{
>> +    ProxyMemoryRegion *pmr = opaque;
>> +
>> +    send_bar_access_msg(pmr->dev, &pmr->mr, true, addr, &val, size,
>> +                        pmr->memory);
>> +}
>> +
>> +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;
>> +}
>> +
>> +const MemoryRegionOps proxy_default_ops = {
>> +    .read = proxy_default_bar_read,
>> +    .write = proxy_default_bar_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
>> +    },
>> +};
>> diff --git a/include/hw/proxy/qemu-proxy.h b/include/hw/proxy/qemu-proxy.h
>> index 29fa2e9..44e370e 100644
>> --- a/include/hw/proxy/qemu-proxy.h
>> +++ b/include/hw/proxy/qemu-proxy.h
>> @@ -22,7 +22,19 @@
>>   #define PCI_PROXY_DEV_GET_CLASS(obj) \
>>               OBJECT_GET_CLASS(PCIProxyDevClass, (obj), TYPE_PCI_PROXY_DEV)
>>   
>> -typedef struct PCIProxyDev {
>> +typedef struct PCIProxyDev PCIProxyDev;
>> +
>> +typedef struct ProxyMemoryRegion {
>> +    PCIProxyDev *dev;
>> +    MemoryRegion mr;
>> +    bool memory;
>> +    bool present;
>> +    uint8_t type;
>> +} ProxyMemoryRegion;
>> +
>> +extern const MemoryRegionOps proxy_default_ops;
>> +
>> +struct PCIProxyDev {
>>       PCIDevice parent_dev;
>>   
>>       MPQemuLinkState *mpqemu_link;
>> @@ -41,7 +53,8 @@ typedef struct PCIProxyDev {
>>       void (*init_proxy) (PCIDevice *dev, char *command, char *exec_name,
>>                           bool need_spawn, Error **errp);
>>   
>> -} PCIProxyDev;
>> +    ProxyMemoryRegion region[PCI_NUM_REGIONS];
>> +};
>>   
>>   typedef struct PCIProxyDevClass {
>>       PCIDeviceClass parent_class;
>> @@ -51,4 +64,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);
>> +
>>   #endif /* QEMU_PROXY_H */
>> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
>> index 5a2be48..1a7738e 100644
>> --- a/include/io/mpqemu-link.h
>> +++ b/include/io/mpqemu-link.h
>> @@ -38,6 +38,8 @@
>>    * PCI_CONFIG_READ        PCI configuration space read
>>    * PCI_CONFIG_WRITE       PCI configuration space write
>>    * SYNC_SYSMEM      Shares QEMU's RAM with remote device's RAM
>> + * BAR_WRITE        Writes to PCI BAR region
>> + * BAR_READ         Reads from PCI BAR region
>>    *
>>    * proc_cmd_t enum type to specify the command to be executed on the remote
>>    * device.
>> @@ -47,6 +49,8 @@ typedef enum {
>>       PCI_CONFIG_READ,
>>       PCI_CONFIG_WRITE,
>>       SYNC_SYSMEM,
>> +    BAR_WRITE,
>> +    BAR_READ,
>>       MAX,
>>   } mpqemu_cmd_t;
>>   
>> @@ -70,6 +74,13 @@ typedef struct {
>>   } sync_sysmem_msg_t;
>>   
>>   typedef struct {
>> +    hwaddr addr;
>> +    uint64_t val;
>> +    unsigned size;
>> +    bool memory;
>> +} bar_access_msg_t;
>> +
>> +typedef struct {
>>       mpqemu_cmd_t cmd;
>>       int bytestream;
>>       size_t size;
>> @@ -77,6 +88,7 @@ typedef struct {
>>       union {
>>           uint64_t u64;
>>           sync_sysmem_msg_t sync_sysmem;
>> +        bar_access_msg_t bar_access;
>>       } data1;
>>   
>>       int fds[REMOTE_MAX_FDS];
>> diff --git a/remote/remote-main.c b/remote/remote-main.c
>> index 7b4cf2f..acd8daf 100644
>> --- a/remote/remote-main.c
>> +++ b/remote/remote-main.c
>> @@ -33,6 +33,7 @@
>>   #include "sysemu/sysemu.h"
>>   #include "block/block.h"
>>   #include "exec/ramlist.h"
>> +#include "exec/memattrs.h"
>>   
>>   static MPQemuLinkState *mpqemu_link;
>>   PCIDevice *remote_pci_dev;
>> @@ -63,6 +64,66 @@ static void process_config_read(MPQemuMsg *msg)
>>       PUT_REMOTE_WAIT(wait);
>>   }
>>   
>> +/* 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;
>> +
>> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
>> +                           (uint8_t *)&bar_access->val, bar_access->size, true);
>> +
>> +    if (res != MEMTX_OK) {
>> +        error_setg(errp, "Could not perform address space write operation,"
>> +                   " inaccessible address: %lx.", bar_access->addr);
>> +    }
>> +}
>> +
>> +static void process_bar_read(MPQemuMsg *msg, Error **errp)
>> +{
>> +    bar_access_msg_t *bar_access = &msg->data1.bar_access;
>> +    AddressSpace *as;
>> +    int wait = msg->fds[0];
>> +    MemTxResult res;
>> +    uint64_t val = 0;
>> +
>> +    as = bar_access->memory ? &address_space_memory : &address_space_io;
>> +
>> +    assert(bar_access->size <= sizeof(uint64_t));
> 
> Note you don't have that check on the write function above.
> Do you actually want something like:
>     assert(is_power_of_2(bar_access->size) && bar_access->size <= sizeof(uint64_t));

Thanks, Dave! I think we should add this check to the write function as
well.

> 
>> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
>> +                           (uint8_t *)&val, bar_access->size, false);
>> +
>> +    if (res != MEMTX_OK) {
>> +        error_setg(errp, "Could not perform address space read operation,"
>> +                   " inaccessible address: %lx.", bar_access->addr);
>> +        val = (uint64_t)-1;
>> +        goto fail;
>> +    }
>> +
>> +    switch (bar_access->size) {
> 
> No case 8 ?

You're correct; we need case 8 in this case.

We don't need to do an explicit typecast for case 8, as the variable
"val" is already 8 bytes long. However, if we don't have case 8, then
all 8-byte reads would get picked up as "default" and report an error.

We initially didn't have the "default" case. But checkpatch complained
about switch-case without default, and this bug got introduced
unwittingly.

Thank you!
--
Jag

> 
> Dave
> 
>> +    case 4:
>> +        val = *((uint32_t *)&val);
>> +        break;
>> +    case 2:
>> +        val = *((uint16_t *)&val);
>> +        break;
>> +    case 1:
>> +        val = *((uint8_t *)&val);
>> +        break;
>> +    default:
>> +        error_setg(errp, "Invalid PCI BAR read size");
>> +        return;
>> +    }
>> +
>> +fail:
>> +    notify_proxy(wait, val);
>> +
>> +    PUT_REMOTE_WAIT(wait);
>> +}
>> +
>>   static void process_msg(GIOCondition cond, MPQemuChannel *chan)
>>   {
>>       MPQemuMsg *msg = NULL;
>> @@ -88,6 +149,18 @@ static void process_msg(GIOCondition cond, MPQemuChannel *chan)
>>       case PCI_CONFIG_READ:
>>           process_config_read(msg);
>>           break;
>> +    case BAR_WRITE:
>> +        process_bar_write(msg, &err);
>> +        if (err) {
>> +            goto finalize_loop;
>> +        }
>> +        break;
>> +    case BAR_READ:
>> +        process_bar_read(msg, &err);
>> +        if (err) {
>> +            goto finalize_loop;
>> +        }
>> +        break;
>>       default:
>>           error_setg(&err, "Unknown command");
>>           goto finalize_loop;
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>