Re: [PATCH v11 12/19] multi-process: Forward PCI config space acceses to the remote process

[Philippe Mathieu-Daudé]

On 10/15/20 8:05 PM, Jagannathan Raman wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>
> The Proxy Object sends the PCI config space accesses as messages
> to the remote process over the communication channel
>
> TODO:
> Investigate if the local PCI config writes can be dropped.
> Without the proxy local PCI config space writes for the device,
> the driver in the guest times out on the probing.
> We have tried to only refer to the remote for the PCI config writes,
> but the driver timeout in the guest forced as to left this
> as it is (removing local PCI config only).

I expect this TODO to be lost in git history. Better to
commit it in the code and remove it once resolved IMHO.

> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/i386/remote-msg.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/pci/proxy.c           | 50 ++++++++++++++++++++++++++++++++++++++
>   include/io/mpqemu-link.h |  9 +++++++
>   io/mpqemu-link.c         |  6 +++++
>   4 files changed, 127 insertions(+)

Please have a look at scripts/git.orderfile, it helps
reviewers to avoid scrolling down/up the patches.

> diff --git a/hw/i386/remote-msg.c b/hw/i386/remote-msg.c
> index 6451b77..94937db 100644
> --- a/hw/i386/remote-msg.c
> +++ b/hw/i386/remote-msg.c
> @@ -15,6 +15,12 @@
>   #include "io/mpqemu-link.h"
>   #include "qapi/error.h"
>   #include "sysemu/runstate.h"
> +#include "hw/pci/pci.h"
> +
> +static void process_config_write(QIOChannel *ioc, PCIDevice *dev,
> +                                 MPQemuMsg *msg);
> +static void process_config_read(QIOChannel *ioc, PCIDevice *dev,
> +                                MPQemuMsg *msg);
>   
>   void coroutine_fn mpqemu_remote_msg_loop_co(void *data)
>   {
> @@ -43,6 +49,12 @@ void coroutine_fn mpqemu_remote_msg_loop_co(void *data)
>           }
>   
>           switch (msg.cmd) {
> +        case PCI_CONFIG_WRITE:
> +            process_config_write(com->ioc, pci_dev, &msg);
> +            break;
> +        case PCI_CONFIG_READ:
> +            process_config_read(com->ioc, pci_dev, &msg);
> +            break;
>           default:
>               error_setg(&local_err,
>                          "Unknown command (%d) received for device %s (pid=%d)",
> @@ -60,3 +72,53 @@ void coroutine_fn mpqemu_remote_msg_loop_co(void *data)
>   
>       return;
>   }
> +
> +static void process_config_write(QIOChannel *ioc, PCIDevice *dev,
> +                                 MPQemuMsg *msg)
> +{
> +    ConfDataMsg *conf = (ConfDataMsg *)&msg->data.conf_data;
> +    MPQemuMsg ret = { 0 };
> +    Error *local_err = NULL;
> +
> +    if ((conf->addr + sizeof(conf->val)) > pci_config_size(dev)) {
> +        error_report("Bad address received when writing PCI config, pid %d",
> +                     getpid());
> +        ret.data.u64 = UINT64_MAX;
> +    } else {
> +        pci_default_write_config(dev, conf->addr, conf->val, conf->l);
> +    }
> +
> +    ret.cmd = RET_MSG;
> +    ret.size = sizeof(ret.data.u64);
> +
> +    mpqemu_msg_send(&ret, ioc, &local_err);
> +    if (local_err) {
> +        error_report("Could not send message to proxy from pid %d",
> +                     getpid());
> +    }
> +}
> +
> +static void process_config_read(QIOChannel *ioc, PCIDevice *dev,
> +                                MPQemuMsg *msg)
> +{
> +    ConfDataMsg *conf = (ConfDataMsg *)&msg->data.conf_data;
> +    MPQemuMsg ret = { 0 };
> +    Error *local_err = NULL;
> +
> +    if ((conf->addr + sizeof(conf->val)) > pci_config_size(dev)) {
> +        error_report("Bad address received when reading PCI config, pid %d",
> +                     getpid());
> +        ret.data.u64 = UINT64_MAX;
> +    } else {
> +        ret.data.u64 = pci_default_read_config(dev, conf->addr, conf->l);

Isn't it endianess issue here? It might makes sense to
declare ConfDataMsg and PCI_CONFIG_READ/PCI_CONFIG_WRITE
packets in little endian.

> +    }
> +
> +    ret.cmd = RET_MSG;
> +    ret.size = sizeof(ret.data.u64);
> +
> +    mpqemu_msg_send(&ret, ioc, &local_err);
> +    if (local_err) {
> +        error_report("Could not send message to proxy from pid %d",
> +                     getpid());
> +    }
> +}
> diff --git a/hw/pci/proxy.c b/hw/pci/proxy.c
> index 7a12f23..27714fe 100644
> --- a/hw/pci/proxy.c
> +++ b/hw/pci/proxy.c
> @@ -16,6 +16,8 @@
>   #include "hw/qdev-properties.h"
>   #include "monitor/monitor.h"
>   #include "migration/blocker.h"
> +#include "io/mpqemu-link.h"
> +#include "qemu/error-report.h"
>   
>   static void proxy_set_socket(PCIProxyDev *pdev, int fd, Error **errp)
>   {
> @@ -69,6 +71,51 @@ static void pci_proxy_dev_exit(PCIDevice *pdev)
>       error_free(dev->migration_blocker);
>   }
>   
> +static int config_op_send(PCIProxyDev *pdev, uint32_t addr, uint32_t *val,
> +                          int l, unsigned int op)
> +{
> +    MPQemuMsg msg = { 0 };
> +    uint64_t ret = -EINVAL;
> +    Error *local_err = NULL;
> +
> +    msg.cmd = op;
> +    msg.data.conf_data.addr = addr;
> +    msg.data.conf_data.val = (op == PCI_CONFIG_WRITE) ? *val : 0;
> +    msg.data.conf_data.l = l;
> +    msg.size = sizeof(ConfDataMsg);
> +
> +    ret = mpqemu_msg_send_and_await_reply(&msg, pdev, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }
> +    if (op == PCI_CONFIG_READ) {
> +        *val = (uint32_t)ret;
> +    }
> +
> +    return ret;
> +}
> +
> +static uint32_t pci_proxy_read_config(PCIDevice *d, uint32_t addr, int len)
> +{
> +    uint32_t val;
> +
> +    (void)config_op_send(PCI_PROXY_DEV(d), addr, &val, len, PCI_CONFIG_READ);
> +
> +    return val;
> +}
> +
> +static void pci_proxy_write_config(PCIDevice *d, uint32_t addr, uint32_t val,
> +                                   int l)
> +{
> +    /*
> +     * Some of the functions access the copy of the remote device
> +     * PCI config space, therefore maintain it updated.
> +     */
> +    pci_default_write_config(d, addr, val, l);
> +
> +    (void)config_op_send(PCI_PROXY_DEV(d), addr, &val, l, PCI_CONFIG_WRITE);
> +}
> +
>   static void pci_proxy_dev_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -76,6 +123,9 @@ static void pci_proxy_dev_class_init(ObjectClass *klass, 
> void *data)
>   
>       k->realize = pci_proxy_dev_realize;
>       k->exit = pci_proxy_dev_exit;
> +    k->config_read = pci_proxy_read_config;
> +    k->config_write = pci_proxy_write_config;
> +
>       device_class_set_props(dc, proxy_properties);
>   }
>   
> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> index 4bea5da..459d345 100644
> --- a/include/io/mpqemu-link.h
> +++ b/include/io/mpqemu-link.h
> @@ -34,6 +34,8 @@ typedef enum {
>       MPQEMU_CMD_INIT,
>       SYNC_SYSMEM,
>       RET_MSG,
> +    PCI_CONFIG_WRITE,
> +    PCI_CONFIG_READ,
>       MPQEMU_CMD_MAX,
>   } MPQemuCmd;
>   
> @@ -43,6 +45,12 @@ typedef struct {
>       off_t offsets[REMOTE_MAX_FDS];
>   } SyncSysmemMsg;
>   
> +typedef struct {
> +    uint32_t addr;
> +    uint32_t val;
> +    int l;
> +} ConfDataMsg;

Maybe name this PciConfDataMsg?

> +
>   /**
>    * MPQemuMsg:
>    * @cmd: The remote command
> @@ -60,6 +68,7 @@ typedef struct {
>   
>       union {
>           uint64_t u64;
> +        ConfDataMsg conf_data;
>           SyncSysmemMsg sync_sysmem;
>       } data;
>   
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index 1339cc7..5ef82da 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -278,6 +278,12 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
>               return false;
>           }
>           break;
> +    case PCI_CONFIG_WRITE:
> +    case PCI_CONFIG_READ:
> +        if (msg->size != sizeof(ConfDataMsg)) {
> +            return false;
> +        }
> +        break;
>       default:
>           break;
>       }