| Message ID | 20170316112030.20419-17-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 03/16/2017 11:20 AM, Andre Przywara wrote: > This introduces the ITS command handler for the CLEAR command, which > clears the pending state of an LPI. > This removes a not-yet injected, but already queued IRQ from a VCPU. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++-- > 1 file changed, 33 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index 267a573..e808f43 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -131,8 +131,8 @@ static void put_devid_evid(struct virt_its *its, struct vits_itte *itte) > * protect the ITTs with their less-than-page-size granularity. > * Takes and drops the its_lock. > */ > -bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid, > - struct vcpu **vcpu, uint32_t *vlpi) > +static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid, NIT: Please mention in the commit message why you turned those functions to static. > + struct vcpu **vcpu, uint32_t *vlpi) > { > struct vits_itte *itte; > int collid; > @@ -216,6 +216,34 @@ static uint64_t its_cmd_mask_field(uint64_t *its_cmd, > #define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32) > #define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1) > > +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr) > +{ > + uint32_t devid = its_cmd_get_deviceid(cmdptr); > + uint32_t eventid = its_cmd_get_id(cmdptr); > + struct pending_irq *pirq; > + struct vcpu *vcpu; > + uint32_t vlpi; > + > + if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) ) > + return -1; > + > + /* Remove a pending, but not yet injected guest IRQ. */ Copying Stefano's comment from last year: "We need to check that the vlpi hasn't already been added to an LR register. We can do that with GIC_IRQ_GUEST_VISIBLE. In case GIC_IRQ_GUEST_VISIBLE is set, we need to clear the lr (clear_lr). If we don't handle this case, we should at least print a warning." > + pirq = lpi_to_pending(vcpu, vlpi, false); > + if ( pirq ) > + { > + clear_bit(GIC_IRQ_GUEST_QUEUED, &pirq->status); > + gic_remove_from_queues(vcpu, vlpi); > + > + /* Mark this pending IRQ struct as availabe again. */ > + if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) ) > + pirq->irq = 0; > + } > + > + clear_bit(vlpi - LPI_OFFSET, vcpu->arch.vgic.pendtable); > + > + return 0; > +} > + > #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) > > static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, > @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, > cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); > switch (its_cmd_get_command(cmdptr)) > { > + case GITS_CMD_CLEAR: > + its_handle_clear(its, cmdptr); Should not you check the return for its_handle_clear? > + break; > case GITS_CMD_SYNC: > /* We handle ITS commands synchronously, so we ignore SYNC. */ > break; > Regards,
Hi, On 24/03/17 14:27, Julien Grall wrote: > Hi Andre, > > On 03/16/2017 11:20 AM, Andre Przywara wrote: >> This introduces the ITS command handler for the CLEAR command, which >> clears the pending state of an LPI. >> This removes a not-yet injected, but already queued IRQ from a VCPU. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++-- >> 1 file changed, 33 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >> index 267a573..e808f43 100644 >> --- a/xen/arch/arm/vgic-v3-its.c >> +++ b/xen/arch/arm/vgic-v3-its.c [....] >> @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d, >> struct virt_its *its, >> cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); >> switch (its_cmd_get_command(cmdptr)) >> { >> + case GITS_CMD_CLEAR: >> + its_handle_clear(its, cmdptr); > > Should not you check the return for its_handle_clear? That sounds obvious, but actually I don't know of a good way of handling this. Blame the architecture, if you like. Passing the error value up would end up in the MMIO handler, where it is not correct to return an error (since the CWRITER MMIO access itself was successful). So I picked one of the behaviors described in 6.3.2 "Command errors", which is simply to ignore the command. If we have a nice way of injecting an SError (do we?), I _could_ check GITS_TYPER.SEIS and then inject it. But effectively this would be untested code, since Linux does not use this feature. So any idea here? I don't think the typical Xen answer of "Crash the guest!" is compliant with the architecture, which leaves three choices, and setting the box on fire is not one of them. That's why I chose to ignore the return value at this point, but at least generate the error condition internally and bail out early. At least for the Tech Preview Dom0 edition of the ITS emulation. If we later gain a good method of handling (and testing!) command errors, we can easily add them. Cheers, Andre. > >> + break; >> case GITS_CMD_SYNC: >> /* We handle ITS commands synchronously, so we ignore >> SYNC. */ >> break; >> > > Regards, >
On Fri, 24 Mar 2017, Andre Przywara wrote: > Hi, > > On 24/03/17 14:27, Julien Grall wrote: > > Hi Andre, > > > > On 03/16/2017 11:20 AM, Andre Przywara wrote: > >> This introduces the ITS command handler for the CLEAR command, which > >> clears the pending state of an LPI. > >> This removes a not-yet injected, but already queued IRQ from a VCPU. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> --- > >> xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++-- > >> 1 file changed, 33 insertions(+), 2 deletions(-) > >> > >> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > >> index 267a573..e808f43 100644 > >> --- a/xen/arch/arm/vgic-v3-its.c > >> +++ b/xen/arch/arm/vgic-v3-its.c > > [....] > > >> @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d, > >> struct virt_its *its, > >> cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); > >> switch (its_cmd_get_command(cmdptr)) > >> { > >> + case GITS_CMD_CLEAR: > >> + its_handle_clear(its, cmdptr); > > > > Should not you check the return for its_handle_clear? > > That sounds obvious, but actually I don't know of a good way of handling > this. Blame the architecture, if you like. Passing the error value up > would end up in the MMIO handler, where it is not correct to return an > error (since the CWRITER MMIO access itself was successful). > > So I picked one of the behaviors described in 6.3.2 "Command errors", > which is simply to ignore the command. > If we have a nice way of injecting an SError (do we?), We do with Wei's series, which is very likely to go in before this one: http://marc.info/?l=xen-devel&m=148940261914581 In particular, see 1489402563-4978-7-git-send-email-Wei.Chen@arm.com. > I _could_ check GITS_TYPER.SEIS and then inject it. But effectively > this would be untested code, since Linux does not use this feature. Keep in mind that Xen supports a wide range of OSes. GITS_TYPER is emulated by Xen in the virtual ITS, right? If so, it doesn't matter the hardware value of SEIS, we can set the virtual value to 1. > So any idea here? I don't think the typical Xen answer of "Crash the > guest!" is compliant with the architecture, which leaves three choices, > and setting the box on fire is not one of them. > > That's why I chose to ignore the return value at this point, but at > least generate the error condition internally and bail out early. At > least for the Tech Preview Dom0 edition of the ITS emulation. > If we later gain a good method of handling (and testing!) command > errors, we can easily add them. At the very least, we need to print a warning (rate limited, so probably gdprintk). All errors that cannot be handled otherwise need to result in a warning. Sending an SError would be fine, I think.
Hi, On 24/03/17 17:17, Stefano Stabellini wrote: > On Fri, 24 Mar 2017, Andre Przywara wrote: >> Hi, >> >> On 24/03/17 14:27, Julien Grall wrote: >>> Hi Andre, >>> >>> On 03/16/2017 11:20 AM, Andre Przywara wrote: >>>> This introduces the ITS command handler for the CLEAR command, which >>>> clears the pending state of an LPI. >>>> This removes a not-yet injected, but already queued IRQ from a VCPU. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 33 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >>>> index 267a573..e808f43 100644 >>>> --- a/xen/arch/arm/vgic-v3-its.c >>>> +++ b/xen/arch/arm/vgic-v3-its.c >> >> [....] >> >>>> @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d, >>>> struct virt_its *its, >>>> cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); >>>> switch (its_cmd_get_command(cmdptr)) >>>> { >>>> + case GITS_CMD_CLEAR: >>>> + its_handle_clear(its, cmdptr); >>> >>> Should not you check the return for its_handle_clear? >> >> That sounds obvious, but actually I don't know of a good way of handling >> this. Blame the architecture, if you like. Passing the error value up >> would end up in the MMIO handler, where it is not correct to return an >> error (since the CWRITER MMIO access itself was successful). >> >> So I picked one of the behaviors described in 6.3.2 "Command errors", >> which is simply to ignore the command. >> If we have a nice way of injecting an SError (do we?), > > We do with Wei's series, which is very likely to go in before this one: > > http://marc.info/?l=xen-devel&m=148940261914581 > > In particular, see 1489402563-4978-7-git-send-email-Wei.Chen@arm.com. > > >> I _could_ check GITS_TYPER.SEIS and then inject it. But effectively >> this would be untested code, since Linux does not use this feature. > > Keep in mind that Xen supports a wide range of OSes. I clearly understand that and don't question it. What I wanted to point out is that using an SError to signal ITS errors is mostly uncharted territory (see the current discussion about handling SErrors in Linux[1]). So we would add code that cannot be tested. And given the current situation and the tech preview status of the ITS support I'd prefer to not go there at the moment. I would offer to annotate the error returns with the actual ITS error codes (as in the KVM code, for instance [2]). Then put a comment in the code explaining the missing error signalling situation, and we create a ticket to notify ourselves of fixing this in the future. Does that make sense? > GITS_TYPER is > emulated by Xen in the virtual ITS, right? If so, it doesn't matter the > hardware value of SEIS, we can set the virtual value to 1. > > >> So any idea here? I don't think the typical Xen answer of "Crash the >> guest!" is compliant with the architecture, which leaves three choices, >> and setting the box on fire is not one of them. >> >> That's why I chose to ignore the return value at this point, but at >> least generate the error condition internally and bail out early. At >> least for the Tech Preview Dom0 edition of the ITS emulation. >> If we later gain a good method of handling (and testing!) command >> errors, we can easily add them. > > At the very least, we need to print a warning (rate limited, so probably > gdprintk). All errors that cannot be handled otherwise need to result in > a warning. OK, I will do this. > Sending an SError would be fine, I think. As mentioned above, I'd refrain from it at the moment. Cheers, Andre. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/496722.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/virt/kvm/arm/vgic/vgic-its.c#n561
Hi Andre, On 27/03/17 09:44, Andre Przywara wrote: > Hi, > > On 24/03/17 17:17, Stefano Stabellini wrote: >> On Fri, 24 Mar 2017, Andre Przywara wrote: >>> Hi, >>> >>> On 24/03/17 14:27, Julien Grall wrote: >>>> Hi Andre, >>>> >>>> On 03/16/2017 11:20 AM, Andre Przywara wrote: >>>>> This introduces the ITS command handler for the CLEAR command, which >>>>> clears the pending state of an LPI. >>>>> This removes a not-yet injected, but already queued IRQ from a VCPU. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>>> --- >>>>> xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 33 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >>>>> index 267a573..e808f43 100644 >>>>> --- a/xen/arch/arm/vgic-v3-its.c >>>>> +++ b/xen/arch/arm/vgic-v3-its.c >>> >>> [....] >>> >>>>> @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d, >>>>> struct virt_its *its, >>>>> cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); >>>>> switch (its_cmd_get_command(cmdptr)) >>>>> { >>>>> + case GITS_CMD_CLEAR: >>>>> + its_handle_clear(its, cmdptr); >>>> >>>> Should not you check the return for its_handle_clear? >>> >>> That sounds obvious, but actually I don't know of a good way of handling >>> this. Blame the architecture, if you like. Passing the error value up >>> would end up in the MMIO handler, where it is not correct to return an >>> error (since the CWRITER MMIO access itself was successful). >>> >>> So I picked one of the behaviors described in 6.3.2 "Command errors", >>> which is simply to ignore the command. >>> If we have a nice way of injecting an SError (do we?), >> >> We do with Wei's series, which is very likely to go in before this one: >> >> http://marc.info/?l=xen-devel&m=148940261914581 >> >> In particular, see 1489402563-4978-7-git-send-email-Wei.Chen@arm.com. >> >> >>> I _could_ check GITS_TYPER.SEIS and then inject it. But effectively >>> this would be untested code, since Linux does not use this feature. >> >> Keep in mind that Xen supports a wide range of OSes. > > I clearly understand that and don't question it. What I wanted to point > out is that using an SError to signal ITS errors is mostly uncharted > territory (see the current discussion about handling SErrors in > Linux[1]). So we would add code that cannot be tested. The SError is sent or not, it is not important whether the OS is able to handle it or not. And justifying with "the code cannot be tested" is not argument as I would have expected you to hack Linux to exercise the vITS. > And given the > current situation and the tech preview status of the ITS support I'd > prefer to not go there at the moment. > > I would offer to annotate the error returns with the actual ITS error > codes (as in the KVM code, for instance [2]). I am happy with that as a first step. > Then put a comment in the code explaining the missing error signalling > situation, and we create a ticket to notify ourselves of fixing this in > the future. I think we still need to log the error. It is useful for the user to know what's going on. You could imagine someone trying to implement an OS using Xen and KVM. He would be really grateful to know what's going on. Cheers,
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index 267a573..e808f43 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -131,8 +131,8 @@ static void put_devid_evid(struct virt_its *its, struct vits_itte *itte) * protect the ITTs with their less-than-page-size granularity. * Takes and drops the its_lock. */ -bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid, - struct vcpu **vcpu, uint32_t *vlpi) +static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid, + struct vcpu **vcpu, uint32_t *vlpi) { struct vits_itte *itte; int collid; @@ -216,6 +216,34 @@ static uint64_t its_cmd_mask_field(uint64_t *its_cmd, #define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32) #define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1) +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr) +{ + uint32_t devid = its_cmd_get_deviceid(cmdptr); + uint32_t eventid = its_cmd_get_id(cmdptr); + struct pending_irq *pirq; + struct vcpu *vcpu; + uint32_t vlpi; + + if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) ) + return -1; + + /* Remove a pending, but not yet injected guest IRQ. */ + pirq = lpi_to_pending(vcpu, vlpi, false); + if ( pirq ) + { + clear_bit(GIC_IRQ_GUEST_QUEUED, &pirq->status); + gic_remove_from_queues(vcpu, vlpi); + + /* Mark this pending IRQ struct as availabe again. */ + if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) ) + pirq->irq = 0; + } + + clear_bit(vlpi - LPI_OFFSET, vcpu->arch.vgic.pendtable); + + return 0; +} + #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); switch (its_cmd_get_command(cmdptr)) { + case GITS_CMD_CLEAR: + its_handle_clear(its, cmdptr); + break; case GITS_CMD_SYNC: /* We handle ITS commands synchronously, so we ignore SYNC. */ break;
This introduces the ITS command handler for the CLEAR command, which clears the pending state of an LPI. This removes a not-yet injected, but already queued IRQ from a VCPU. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-)