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
>