| Message ID | 20170511175340.8448-24-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 11/05/17 18:53, Andre Przywara wrote: > The DISCARD command drops the connection between a DeviceID/EventID > and an LPI/collection pair. > We mark the respective structure entries as not allocated and make > sure that any queued IRQs are removed. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3-its.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index ef7c78f..f7a8d77 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -723,6 +723,27 @@ out_unlock: > return ret; > } > > +static int its_handle_discard(struct virt_its *its, uint64_t *cmdptr) > +{ > + uint32_t devid = its_cmd_get_deviceid(cmdptr); > + uint32_t eventid = its_cmd_get_id(cmdptr); > + int ret; > + > + spin_lock(&its->its_lock); > + > + /* Remove from the radix tree and remove the host entry. */ > + ret = its_discard_event(its, devid, eventid); > + > + /* Remove from the guest's ITTE. */ > + if ( ret || write_itte_locked(its, devid, eventid, > + UNMAPPED_COLLECTION, INVALID_LPI, NULL) ) I am not sure to fully understand this if. If ret is not NULL you override it and never call write_itte_locked. Is it what you wanted? If so, then probably a bit more documentation would be useful to explain why writte_itte_locked is skipped. > + ret = -1; > + > + spin_unlock(&its->its_lock); > + > + return ret; > +} > + > #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) > #define ITS_CMD_OFFSET(reg) ((reg) & GENMASK(19, 5)) > > @@ -755,6 +776,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its) > case GITS_CMD_CLEAR: > ret = its_handle_clear(its, command); > break; > + case GITS_CMD_DISCARD: > + ret = its_handle_discard(its, command); > + break; > case GITS_CMD_INT: > ret = its_handle_int(its, command); > break; > Cheers,
Hi, On 18/05/17 15:23, Julien Grall wrote: > Hi Andre, > > On 11/05/17 18:53, Andre Przywara wrote: >> The DISCARD command drops the connection between a DeviceID/EventID >> and an LPI/collection pair. >> We mark the respective structure entries as not allocated and make >> sure that any queued IRQs are removed. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic-v3-its.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >> index ef7c78f..f7a8d77 100644 >> --- a/xen/arch/arm/vgic-v3-its.c >> +++ b/xen/arch/arm/vgic-v3-its.c >> @@ -723,6 +723,27 @@ out_unlock: >> return ret; >> } >> >> +static int its_handle_discard(struct virt_its *its, uint64_t *cmdptr) >> +{ >> + uint32_t devid = its_cmd_get_deviceid(cmdptr); >> + uint32_t eventid = its_cmd_get_id(cmdptr); >> + int ret; >> + >> + spin_lock(&its->its_lock); >> + >> + /* Remove from the radix tree and remove the host entry. */ >> + ret = its_discard_event(its, devid, eventid); >> + >> + /* Remove from the guest's ITTE. */ >> + if ( ret || write_itte_locked(its, devid, eventid, >> + UNMAPPED_COLLECTION, INVALID_LPI, >> NULL) ) > > I am not sure to fully understand this if. If ret is not NULL you > override it and never call write_itte_locked. If its_discard_event() succeeded above, then ret will be 0, in which case we call write_itte_locked(). If that returns non-zero, this is an error and we set ret to -1, otherwise (no error) it stays at zero. > Is it what you wanted? If so, then probably a bit more documentation > would be useful to explain why writte_itte_locked is skipped. I admit this is a bit convoluted. I will either document this or simplify the algorithm. Cheers, Andre. > >> + ret = -1; >> + >> + spin_unlock(&its->its_lock); >> + >> + return ret; >> +} >> + >> #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) >> #define ITS_CMD_OFFSET(reg) ((reg) & GENMASK(19, 5)) >> >> @@ -755,6 +776,9 @@ static int vgic_its_handle_cmds(struct domain *d, >> struct virt_its *its) >> case GITS_CMD_CLEAR: >> ret = its_handle_clear(its, command); >> break; >> + case GITS_CMD_DISCARD: >> + ret = its_handle_discard(its, command); >> + break; >> case GITS_CMD_INT: >> ret = its_handle_int(its, command); >> break; >> > > Cheers, >
On 22/05/17 17:50, Andre Przywara wrote: > Hi, Hi Andre, > On 18/05/17 15:23, Julien Grall wrote: >> Hi Andre, >> >> On 11/05/17 18:53, Andre Przywara wrote: >>> The DISCARD command drops the connection between a DeviceID/EventID >>> and an LPI/collection pair. >>> We mark the respective structure entries as not allocated and make >>> sure that any queued IRQs are removed. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> xen/arch/arm/vgic-v3-its.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >>> index ef7c78f..f7a8d77 100644 >>> --- a/xen/arch/arm/vgic-v3-its.c >>> +++ b/xen/arch/arm/vgic-v3-its.c >>> @@ -723,6 +723,27 @@ out_unlock: >>> return ret; >>> } >>> >>> +static int its_handle_discard(struct virt_its *its, uint64_t *cmdptr) >>> +{ >>> + uint32_t devid = its_cmd_get_deviceid(cmdptr); >>> + uint32_t eventid = its_cmd_get_id(cmdptr); >>> + int ret; >>> + >>> + spin_lock(&its->its_lock); >>> + >>> + /* Remove from the radix tree and remove the host entry. */ >>> + ret = its_discard_event(its, devid, eventid); >>> + >>> + /* Remove from the guest's ITTE. */ >>> + if ( ret || write_itte_locked(its, devid, eventid, >>> + UNMAPPED_COLLECTION, INVALID_LPI, >>> NULL) ) >> >> I am not sure to fully understand this if. If ret is not NULL you >> override it and never call write_itte_locked. > > If its_discard_event() succeeded above, then ret will be 0, in which > case we call write_itte_locked(). If that returns non-zero, this is an > error and we set ret to -1, otherwise (no error) it stays at zero. But we want to carry the error from its_discard_event. No? Cheers,
Hi, On 22/05/17 18:20, Julien Grall wrote: > > > On 22/05/17 17:50, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 18/05/17 15:23, Julien Grall wrote: >>> Hi Andre, >>> >>> On 11/05/17 18:53, Andre Przywara wrote: >>>> The DISCARD command drops the connection between a DeviceID/EventID >>>> and an LPI/collection pair. >>>> We mark the respective structure entries as not allocated and make >>>> sure that any queued IRQs are removed. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> xen/arch/arm/vgic-v3-its.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >>>> index ef7c78f..f7a8d77 100644 >>>> --- a/xen/arch/arm/vgic-v3-its.c >>>> +++ b/xen/arch/arm/vgic-v3-its.c >>>> @@ -723,6 +723,27 @@ out_unlock: >>>> return ret; >>>> } >>>> >>>> +static int its_handle_discard(struct virt_its *its, uint64_t *cmdptr) >>>> +{ >>>> + uint32_t devid = its_cmd_get_deviceid(cmdptr); >>>> + uint32_t eventid = its_cmd_get_id(cmdptr); >>>> + int ret; >>>> + >>>> + spin_lock(&its->its_lock); >>>> + >>>> + /* Remove from the radix tree and remove the host entry. */ >>>> + ret = its_discard_event(its, devid, eventid); >>>> + >>>> + /* Remove from the guest's ITTE. */ >>>> + if ( ret || write_itte_locked(its, devid, eventid, >>>> + UNMAPPED_COLLECTION, INVALID_LPI, >>>> NULL) ) >>> >>> I am not sure to fully understand this if. If ret is not NULL you >>> override it and never call write_itte_locked. >> >> If its_discard_event() succeeded above, then ret will be 0, in which >> case we call write_itte_locked(). If that returns non-zero, this is an >> error and we set ret to -1, otherwise (no error) it stays at zero. > > But we want to carry the error from its_discard_event. No? Ah right, this was so cleverly made that I even tricked myself ;-) I changed this now to be more easily readable. Cheers, Andre.
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index ef7c78f..f7a8d77 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -723,6 +723,27 @@ out_unlock: return ret; } +static int its_handle_discard(struct virt_its *its, uint64_t *cmdptr) +{ + uint32_t devid = its_cmd_get_deviceid(cmdptr); + uint32_t eventid = its_cmd_get_id(cmdptr); + int ret; + + spin_lock(&its->its_lock); + + /* Remove from the radix tree and remove the host entry. */ + ret = its_discard_event(its, devid, eventid); + + /* Remove from the guest's ITTE. */ + if ( ret || write_itte_locked(its, devid, eventid, + UNMAPPED_COLLECTION, INVALID_LPI, NULL) ) + ret = -1; + + spin_unlock(&its->its_lock); + + return ret; +} + #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) #define ITS_CMD_OFFSET(reg) ((reg) & GENMASK(19, 5)) @@ -755,6 +776,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its) case GITS_CMD_CLEAR: ret = its_handle_clear(its, command); break; + case GITS_CMD_DISCARD: + ret = its_handle_discard(its, command); + break; case GITS_CMD_INT: ret = its_handle_int(its, command); break;
The DISCARD command drops the connection between a DeviceID/EventID and an LPI/collection pair. We mark the respective structure entries as not allocated and make sure that any queued IRQs are removed. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic-v3-its.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)