| Message ID | 1a1490ba03686fe29a0f627bec1556a0051fed18.1602784930.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for multi-process Qemu | expand |
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; > } >
On 10/23/20 6:59 PM, Philippe Mathieu-Daudé wrote: > 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 ... >> +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. Bah Stefan told me to look at the specs which are included in the latest patch (docs/multi-process.rst). As I process patches of this series in order I haven't looked at it yet. Still, why not put it first in your series? Regards, Phil.
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); + } + + 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; + /** * 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; }