| Message ID | 1541154621-22423-6-git-send-email-pmorel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/vfio: VFIO-AP interrupt control interception | expand |
On Fri, 2 Nov 2018 11:30:21 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > We intercept the PQAP(AQIC) instruction and transform > the guest's AQIC command parameters for the host AQIC > parameters. > > Doing this we use the standard adapter interface to provide > the adapter NIB, indicator and ISC. > > We define a new structure, APQueue to keep track of > the route and indicator address and we add an array of > AP Queues in the VFIOAPDevice. > > We call the VFIO ioctl to set or clear the interruption > according to the "i" bit of the parameter. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/vfio/ap.c | 92 +++++++++++++++++++++++++++++++++++- > include/hw/s390x/ap-device.h | 46 ++++++++++++++++++ > 2 files changed, 137 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c > index d8d9cadc46..67a46e163e 100644 > --- a/hw/vfio/ap.c > +++ b/hw/vfio/ap.c > @@ -27,17 +27,90 @@ > #include "sysemu/sysemu.h" > #include "hw/s390x/ap-bridge.h" > #include "exec/address-spaces.h" > +#include "hw/s390x/s390_flic.h" > +#include "hw/s390x/css.h" > > #define VFIO_AP_DEVICE_TYPE "vfio-ap" > > typedef struct VFIOAPDevice { > APDevice apdev; > VFIODevice vdev; > + QTAILQ_ENTRY(VFIOAPDevice) sibling; > + APQueue apq[MAX_AP][MAX_DOMAIN]; > } VFIOAPDevice; > > #define VFIO_AP_DEVICE(obj) \ > OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) > > +VFIOAPDevice *vfio_apdev; > +static APDevice *matrix; Why do you need those variables? Can't you get the target device in a different way? > + > +static int ap_aqic(CPUS390XState *env) <bikeshed>I think ap_pqap_aqic would be slightly better.</bikeshed> > +{ > + struct pqap_cmd cmd = reg2cmd(env->regs[0]); > + struct ap_status status = reg2status(env->regs[1]); I don't really like those reg2<whatever> helpers; they obfuscate things IMO. Also: These are internal structs and not copied from Linux, right? Then they should be CamelCase. > + uint64_t guest_nib = env->regs[2]; Another point: What about endianness? Even though this is kvm-only, I would like an emulation of an instruction to be endian-clean. (Maybe it also needs a different split?) > + struct vfio_ap_aqic param = {}; > + int retval; > + VFIODevice *vdev; > + VFIOAPDevice *ap_vdev; > + APQueue *apq; > + > + ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, matrix); > + apq = &ap_vdev->apq[cmd.apid][cmd.apqi]; > + vdev = &ap_vdev->vdev; > + > + if (status.irq) { > + if (apq->nib) { > + status.rc = AP_RC_BAD_STATE; > + goto error; > + } > + } else { > + if (!apq->nib) { > + status.rc = AP_RC_BAD_STATE; > + goto error; > + } > + } Maybe if (!!status.irq == !!apq->nib) { status.rc = AP_RC_BAD_STATE; goto error; } ? > + if (!guest_nib) { > + status.rc = AP_RC_INVALID_ADDR; > + goto error; > + } > + > + apq->routes.adapter.adapter_id = css_get_adapter_id( > + CSS_IO_ADAPTER_AP, status.isc); > + > + apq->nib = get_indicator(ldq_p(&guest_nib), 8); > + > + retval = map_indicator(&apq->routes.adapter, apq->nib); > + if (retval) { > + status.rc = AP_RC_INVALID_ADDR; > + env->regs[1] = status2reg(status); I do not like the backwards conversion macros, either :( > + goto error; > + } > + > + param.cmd = env->regs[0]; > + param.status = env->regs[1]; > + param.nib = env->regs[2]; > + param.adapter_id = apq->routes.adapter.adapter_id; > + param.argsz = sizeof(param); > + > + retval = ioctl(vdev->fd, VFIO_AP_SET_IRQ, ¶m); > + status = reg2status(param.status); > + if (retval) { > + goto err_ioctl; > + } > + > + env->regs[1] = param.status; > + > + return 0; > +err_ioctl: > + release_indicator(&apq->routes.adapter, apq->nib); > + apq->nib = NULL; > +error: > + env->regs[1] = status2reg(status); > + return 0; > +} > + > /* > * ap_pqap > * @env: environment pointing to registers > @@ -45,7 +118,20 @@ typedef struct VFIOAPDevice { > */ > int ap_pqap(CPUS390XState *env) > { > - return -PGM_OPERATION; > + struct pqap_cmd cmd = reg2cmd(env->regs[0]); Dito on the macro. > + int cc = 0; > + > + switch (cmd.fc) { This probably needs some endian handling as well. > + case AQIC: What about calling this PQAP_AQCI? > + if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) { > + return -PGM_OPERATION; > + } > + cc = ap_aqic(env); > + break; > + default: > + return -PGM_OPERATION; > + } > + return cc; > } > > static void vfio_ap_compute_needs_reset(VFIODevice *vdev) > @@ -119,6 +205,9 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) > goto out_get_dev_err; > } > > + matrix = apdev; Huh? > + css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false, > + 0, &error_abort); > return; > > out_get_dev_err: > @@ -135,6 +224,7 @@ static void vfio_ap_unrealize(DeviceState *dev, Error **errp) > VFIOGroup *group = vapdev->vdev.group; > > vfio_ap_put_device(vapdev); > + matrix = NULL; > vfio_put_group(group); > } > > diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h > index a83ea096c7..bc2b7bcd8e 100644 > --- a/include/hw/s390x/ap-device.h > +++ b/include/hw/s390x/ap-device.h > @@ -28,4 +28,50 @@ typedef struct APDevice { > #include "cpu.h" > int ap_pqap(CPUS390XState *env); > > +#define MAX_AP 256 > +#define MAX_DOMAIN 256 > + > +#include "hw/s390x/s390_flic.h" > +#include "hw/s390x/css.h" > +typedef struct APQueue { > + uint32_t apid; > + uint32_t apqi; > + AdapterRoutes routes; > + IndAddr *nib; > +} APQueue; > + > +/* AP PQAP commands definitions */ > +#define AQIC 0x03 > + > +struct pqap_cmd { > + uint32_t unused; > + uint8_t fc; > + unsigned t:1; > + unsigned reserved:7; It is better to make this an uint8_t and define an accessor for the 't' bit. > + uint8_t apid; > + uint8_t apqi; > +}; Also, please use typedef and CamelCase :) > +/* AP status returned by the AP PQAP commands */ > +#define AP_RC_APQN_INVALID 0x01 > +#define AP_RC_INVALID_ADDR 0x06 > +#define AP_RC_BAD_STATE 0x07 > + > +struct ap_status { > + uint16_t pad; > + unsigned irq:1; > + unsigned pad2:15; > + unsigned e:1; > + unsigned r:1; > + unsigned f:1; > + unsigned unused:4; > + unsigned i:1; > + unsigned char rc; > + unsigned reserved:13; > + unsigned isc:3; > +}; Dito on bitfields and CamelCase :) > + > +#define reg2cmd(reg) (*(struct pqap_cmd *)&(reg)) > +#define status2reg(status) (*((uint64_t *)&status)) > +#define reg2status(reg) (*(struct ap_status *)&(reg)) > + > #endif /* HW_S390X_AP_DEVICE_H */
On 07/11/2018 14:39, Cornelia Huck wrote: > On Fri, 2 Nov 2018 11:30:21 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> We intercept the PQAP(AQIC) instruction and transform >> the guest's AQIC command parameters for the host AQIC >> parameters. >> >> Doing this we use the standard adapter interface to provide >> the adapter NIB, indicator and ISC. >> >> We define a new structure, APQueue to keep track of >> the route and indicator address and we add an array of >> AP Queues in the VFIOAPDevice. >> >> We call the VFIO ioctl to set or clear the interruption >> according to the "i" bit of the parameter. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/vfio/ap.c | 92 +++++++++++++++++++++++++++++++++++- >> include/hw/s390x/ap-device.h | 46 ++++++++++++++++++ >> 2 files changed, 137 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index d8d9cadc46..67a46e163e 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -27,17 +27,90 @@ >> #include "sysemu/sysemu.h" >> #include "hw/s390x/ap-bridge.h" >> #include "exec/address-spaces.h" >> +#include "hw/s390x/s390_flic.h" >> +#include "hw/s390x/css.h" >> >> #define VFIO_AP_DEVICE_TYPE "vfio-ap" >> >> typedef struct VFIOAPDevice { >> APDevice apdev; >> VFIODevice vdev; >> + QTAILQ_ENTRY(VFIOAPDevice) sibling; >> + APQueue apq[MAX_AP][MAX_DOMAIN]; >> } VFIOAPDevice; >> >> #define VFIO_AP_DEVICE(obj) \ >> OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) >> >> +VFIOAPDevice *vfio_apdev; >> +static APDevice *matrix; > > Why do you need those variables? Can't you get the target device in a > different way? > >> + >> +static int ap_aqic(CPUS390XState *env) > > <bikeshed>I think ap_pqap_aqic would be slightly better.</bikeshed> agreed. > >> +{ >> + struct pqap_cmd cmd = reg2cmd(env->regs[0]); >> + struct ap_status status = reg2status(env->regs[1]); > > I don't really like those reg2<whatever> helpers; they obfuscate things > IMO. > I agree, I will use masks and uint64_t/uint32_t. > Also: These are internal structs and not copied from Linux, right? Then > they should be CamelCase. > >> + uint64_t guest_nib = env->regs[2]; > > Another point: What about endianness? Even though this is kvm-only, I > would like an emulation of an instruction to be endian-clean. (Maybe it > also needs a different split?) OK, I will look at this > >> + struct vfio_ap_aqic param = {}; >> + int retval; >> + VFIODevice *vdev; >> + VFIOAPDevice *ap_vdev; >> + APQueue *apq; >> + >> + ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, matrix); >> + apq = &ap_vdev->apq[cmd.apid][cmd.apqi]; >> + vdev = &ap_vdev->vdev; >> + >> + if (status.irq) { >> + if (apq->nib) { >> + status.rc = AP_RC_BAD_STATE; >> + goto error; >> + } >> + } else { >> + if (!apq->nib) { >> + status.rc = AP_RC_BAD_STATE; >> + goto error; >> + } >> + } > > Maybe > if (!!status.irq == !!apq->nib) { > status.rc = AP_RC_BAD_STATE; > goto error; > } > ? :) > >> + if (!guest_nib) { >> + status.rc = AP_RC_INVALID_ADDR; >> + goto error; >> + } >> + >> + apq->routes.adapter.adapter_id = css_get_adapter_id( >> + CSS_IO_ADAPTER_AP, status.isc); >> + >> + apq->nib = get_indicator(ldq_p(&guest_nib), 8); >> + >> + retval = map_indicator(&apq->routes.adapter, apq->nib); >> + if (retval) { >> + status.rc = AP_RC_INVALID_ADDR; >> + env->regs[1] = status2reg(status); > > I do not like the backwards conversion macros, either :( > >> + goto error; >> + } >> + >> + param.cmd = env->regs[0]; >> + param.status = env->regs[1]; >> + param.nib = env->regs[2]; >> + param.adapter_id = apq->routes.adapter.adapter_id; >> + param.argsz = sizeof(param); >> + >> + retval = ioctl(vdev->fd, VFIO_AP_SET_IRQ, ¶m); >> + status = reg2status(param.status); >> + if (retval) { >> + goto err_ioctl; >> + } >> + >> + env->regs[1] = param.status; >> + >> + return 0; >> +err_ioctl: >> + release_indicator(&apq->routes.adapter, apq->nib); >> + apq->nib = NULL; >> +error: >> + env->regs[1] = status2reg(status); >> + return 0; >> +} >> + >> /* >> * ap_pqap >> * @env: environment pointing to registers >> @@ -45,7 +118,20 @@ typedef struct VFIOAPDevice { >> */ >> int ap_pqap(CPUS390XState *env) >> { >> - return -PGM_OPERATION; >> + struct pqap_cmd cmd = reg2cmd(env->regs[0]); > > Dito on the macro. > >> + int cc = 0; >> + >> + switch (cmd.fc) { > > This probably needs some endian handling as well. > >> + case AQIC: > > What about calling this PQAP_AQCI? I can use it , however the function itself is called ap_pqap() so we already know it is about PQAP... > >> + if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) { >> + return -PGM_OPERATION; >> + } >> + cc = ap_aqic(env); >> + break; >> + default: >> + return -PGM_OPERATION; >> + } >> + return cc; >> } >> >> static void vfio_ap_compute_needs_reset(VFIODevice *vdev) >> @@ -119,6 +205,9 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >> goto out_get_dev_err; >> } >> >> + matrix = apdev; > > Huh? OK, sorry, I can do better. (I hope) > >> + css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false, >> + 0, &error_abort); >> return; >> >> out_get_dev_err: >> @@ -135,6 +224,7 @@ static void vfio_ap_unrealize(DeviceState *dev, Error **errp) >> VFIOGroup *group = vapdev->vdev.group; >> >> vfio_ap_put_device(vapdev); >> + matrix = NULL; >> vfio_put_group(group); >> } >> >> diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h >> index a83ea096c7..bc2b7bcd8e 100644 >> --- a/include/hw/s390x/ap-device.h >> +++ b/include/hw/s390x/ap-device.h >> @@ -28,4 +28,50 @@ typedef struct APDevice { >> #include "cpu.h" >> int ap_pqap(CPUS390XState *env); >> >> +#define MAX_AP 256 >> +#define MAX_DOMAIN 256 >> + >> +#include "hw/s390x/s390_flic.h" >> +#include "hw/s390x/css.h" >> +typedef struct APQueue { >> + uint32_t apid; >> + uint32_t apqi; >> + AdapterRoutes routes; >> + IndAddr *nib; >> +} APQueue; >> + >> +/* AP PQAP commands definitions */ >> +#define AQIC 0x03 >> + >> +struct pqap_cmd { >> + uint32_t unused; >> + uint8_t fc; >> + unsigned t:1; >> + unsigned reserved:7; > > It is better to make this an uint8_t and define an accessor for the 't' > bit. OK > >> + uint8_t apid; >> + uint8_t apqi; >> +}; > > Also, please use typedef and CamelCase :) right, more camels > >> +/* AP status returned by the AP PQAP commands */ >> +#define AP_RC_APQN_INVALID 0x01 >> +#define AP_RC_INVALID_ADDR 0x06 >> +#define AP_RC_BAD_STATE 0x07 >> + >> +struct ap_status { >> + uint16_t pad; >> + unsigned irq:1; >> + unsigned pad2:15; >> + unsigned e:1; >> + unsigned r:1; >> + unsigned f:1; >> + unsigned unused:4; >> + unsigned i:1; >> + unsigned char rc; >> + unsigned reserved:13; >> + unsigned isc:3; >> +}; > > Dito on bitfields and CamelCase :) OK again, even more Camels Thanks, Pierre > >> + >> +#define reg2cmd(reg) (*(struct pqap_cmd *)&(reg)) >> +#define status2reg(status) (*((uint64_t *)&status)) >> +#define reg2status(reg) (*(struct ap_status *)&(reg)) >> + >> #endif /* HW_S390X_AP_DEVICE_H */ >
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index d8d9cadc46..67a46e163e 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -27,17 +27,90 @@ #include "sysemu/sysemu.h" #include "hw/s390x/ap-bridge.h" #include "exec/address-spaces.h" +#include "hw/s390x/s390_flic.h" +#include "hw/s390x/css.h" #define VFIO_AP_DEVICE_TYPE "vfio-ap" typedef struct VFIOAPDevice { APDevice apdev; VFIODevice vdev; + QTAILQ_ENTRY(VFIOAPDevice) sibling; + APQueue apq[MAX_AP][MAX_DOMAIN]; } VFIOAPDevice; #define VFIO_AP_DEVICE(obj) \ OBJECT_CHECK(VFIOAPDevice, (obj), VFIO_AP_DEVICE_TYPE) +VFIOAPDevice *vfio_apdev; +static APDevice *matrix; + +static int ap_aqic(CPUS390XState *env) +{ + struct pqap_cmd cmd = reg2cmd(env->regs[0]); + struct ap_status status = reg2status(env->regs[1]); + uint64_t guest_nib = env->regs[2]; + struct vfio_ap_aqic param = {}; + int retval; + VFIODevice *vdev; + VFIOAPDevice *ap_vdev; + APQueue *apq; + + ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, matrix); + apq = &ap_vdev->apq[cmd.apid][cmd.apqi]; + vdev = &ap_vdev->vdev; + + if (status.irq) { + if (apq->nib) { + status.rc = AP_RC_BAD_STATE; + goto error; + } + } else { + if (!apq->nib) { + status.rc = AP_RC_BAD_STATE; + goto error; + } + } + if (!guest_nib) { + status.rc = AP_RC_INVALID_ADDR; + goto error; + } + + apq->routes.adapter.adapter_id = css_get_adapter_id( + CSS_IO_ADAPTER_AP, status.isc); + + apq->nib = get_indicator(ldq_p(&guest_nib), 8); + + retval = map_indicator(&apq->routes.adapter, apq->nib); + if (retval) { + status.rc = AP_RC_INVALID_ADDR; + env->regs[1] = status2reg(status); + goto error; + } + + param.cmd = env->regs[0]; + param.status = env->regs[1]; + param.nib = env->regs[2]; + param.adapter_id = apq->routes.adapter.adapter_id; + param.argsz = sizeof(param); + + retval = ioctl(vdev->fd, VFIO_AP_SET_IRQ, ¶m); + status = reg2status(param.status); + if (retval) { + goto err_ioctl; + } + + env->regs[1] = param.status; + + return 0; +err_ioctl: + release_indicator(&apq->routes.adapter, apq->nib); + apq->nib = NULL; +error: + env->regs[1] = status2reg(status); + return 0; +} + /* * ap_pqap * @env: environment pointing to registers @@ -45,7 +118,20 @@ typedef struct VFIOAPDevice { */ int ap_pqap(CPUS390XState *env) { - return -PGM_OPERATION; + struct pqap_cmd cmd = reg2cmd(env->regs[0]); + int cc = 0; + + switch (cmd.fc) { + case AQIC: + if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) { + return -PGM_OPERATION; + } + cc = ap_aqic(env); + break; + default: + return -PGM_OPERATION; + } + return cc; } static void vfio_ap_compute_needs_reset(VFIODevice *vdev) @@ -119,6 +205,9 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) goto out_get_dev_err; } + matrix = apdev; + css_register_io_adapters(CSS_IO_ADAPTER_AP, true, false, + 0, &error_abort); return; out_get_dev_err: @@ -135,6 +224,7 @@ static void vfio_ap_unrealize(DeviceState *dev, Error **errp) VFIOGroup *group = vapdev->vdev.group; vfio_ap_put_device(vapdev); + matrix = NULL; vfio_put_group(group); } diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h index a83ea096c7..bc2b7bcd8e 100644 --- a/include/hw/s390x/ap-device.h +++ b/include/hw/s390x/ap-device.h @@ -28,4 +28,50 @@ typedef struct APDevice { #include "cpu.h" int ap_pqap(CPUS390XState *env); +#define MAX_AP 256 +#define MAX_DOMAIN 256 + +#include "hw/s390x/s390_flic.h" +#include "hw/s390x/css.h" +typedef struct APQueue { + uint32_t apid; + uint32_t apqi; + AdapterRoutes routes; + IndAddr *nib; +} APQueue; + +/* AP PQAP commands definitions */ +#define AQIC 0x03 + +struct pqap_cmd { + uint32_t unused; + uint8_t fc; + unsigned t:1; + unsigned reserved:7; + uint8_t apid; + uint8_t apqi; +}; +/* AP status returned by the AP PQAP commands */ +#define AP_RC_APQN_INVALID 0x01 +#define AP_RC_INVALID_ADDR 0x06 +#define AP_RC_BAD_STATE 0x07 + +struct ap_status { + uint16_t pad; + unsigned irq:1; + unsigned pad2:15; + unsigned e:1; + unsigned r:1; + unsigned f:1; + unsigned unused:4; + unsigned i:1; + unsigned char rc; + unsigned reserved:13; + unsigned isc:3; +}; + +#define reg2cmd(reg) (*(struct pqap_cmd *)&(reg)) +#define status2reg(status) (*((uint64_t *)&status)) +#define reg2status(reg) (*(struct ap_status *)&(reg)) + #endif /* HW_S390X_AP_DEVICE_H */
We intercept the PQAP(AQIC) instruction and transform the guest's AQIC command parameters for the host AQIC parameters. Doing this we use the standard adapter interface to provide the adapter NIB, indicator and ISC. We define a new structure, APQueue to keep track of the route and indicator address and we add an array of AP Queues in the VFIOAPDevice. We call the VFIO ioctl to set or clear the interruption according to the "i" bit of the parameter. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- hw/vfio/ap.c | 92 +++++++++++++++++++++++++++++++++++- include/hw/s390x/ap-device.h | 46 ++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-)