| Message ID | 20170526173540.10066-26-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 05/26/2017 06:35 PM, Andre Przywara wrote: > The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU > pair and actually instantiates LPI interrupts. MAPI is just a variant > of this comment, where the LPI ID is the same as the event ID. > We connect the already allocated host LPI to this virtual LPI, so that > any triggering LPI on the host can be quickly forwarded to a guest. > Beside entering the domain and the virtual LPI number in the respective > host LPI entry, we also initialize and add the already allocated > struct pending_irq to our radix tree, so that we can now easily find it > by its virtual LPI number. > We also read the property table to update the enabled bit and the > priority for our new LPI, as we might have missed this during an earlier > INVALL call (which only checks mapped LPIs). But we make sure that the > property table is actually valid, as all redistributors might still > be disabled at this point. > Since write_itte_locked() now sees its first usage, we change the > declaration to static. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-v3-its.c | 27 ++++++++ > xen/arch/arm/vgic-v3-its.c | 138 ++++++++++++++++++++++++++++++++++++++- > xen/include/asm-arm/gic_v3_its.h | 3 + > 3 files changed, 165 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index 8864e0b..41fff64 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d, paddr_t vdoorbell_address, > return 0; > } > > +/* > + * Connects the event ID for an already assigned device to the given VCPU/vLPI > + * pair. The corresponding physical LPI is already mapped on the host side > + * (when assigning the physical device to the guest), so we just connect the > + * target VCPU/vLPI pair to that interrupt to inject it properly if it fires. > + * Returns a pointer to the already allocated struct pending_irq that is > + * meant to be used by that event. > + */ > +struct pending_irq *gicv3_assign_guest_event(struct domain *d, > + paddr_t vdoorbell_address, > + uint32_t vdevid, uint32_t eventid, > + uint32_t virt_lpi) > +{ > + struct pending_irq *pirq; > + uint32_t host_lpi = 0; This should be INVALID_LPI and not 0. [...] > +/* > + * For a given virtual LPI read the enabled bit and priority from the virtual > + * property table and update the virtual IRQ's state in the given pending_irq. > + * Must be called with the respective VGIC VCPU lock held. > + */ > +static int update_lpi_property(struct domain *d, struct pending_irq *p) > +{ > + paddr_t addr; > + uint8_t property; > + int ret; > + > + /* > + * If no redistributor has its LPIs enabled yet, we can't access the > + * property table. In this case we just can't update the properties, > + * but this should not be an error from an ITS point of view. > + */ > + if ( !read_atomic(&d->arch.vgic.rdists_enabled) ) > + return 0; AFAICT, there are no other place where the property would be updated. Should we put a warning to let the user know that property will not be updated? More that you don't return an error so no easy way to know what's going on. > + > + addr = d->arch.vgic.rdist_propbase & GENMASK(51, 12); > + > + ret = vgic_access_guest_memory(d, addr + p->irq - LPI_OFFSET, > + &property, sizeof(property), false); > + if ( ret ) > + return ret; > + > + write_atomic(&p->lpi_priority, property & LPI_PROP_PRIO_MASK); > + > + if ( property & LPI_PROP_ENABLED ) > + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + else > + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + > + return 0; > +} > + > /* Must be called with the ITS lock held. */ > static int its_discard_event(struct virt_its *its, > uint32_t vdevid, uint32_t vevid) > @@ -538,6 +574,98 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr) > return ret; > } > > +static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr) > +{ [...] > + /* > + * radix_tree_insert() returns an error either due to an internal > + * condition (like memory allocation failure) or because the LPI already > + * existed in the tree. We don't support the latter case, so we always > + * cleanup and return an error here in any case. > + */ > +out_remove_host_entry: > + gicv3_remove_guest_event(its->d, its->doorbell_address, devid, eventid); Can gicv3_remove_guest_event fail? If yes, should we check the return/add comment? If not, then we should have an ASSERT(....). Cheers,
Hi, On 02/06/17 18:12, Julien Grall wrote: > Hi Andre, > > On 05/26/2017 06:35 PM, Andre Przywara wrote: >> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU >> pair and actually instantiates LPI interrupts. MAPI is just a variant >> of this comment, where the LPI ID is the same as the event ID. >> We connect the already allocated host LPI to this virtual LPI, so that >> any triggering LPI on the host can be quickly forwarded to a guest. >> Beside entering the domain and the virtual LPI number in the respective >> host LPI entry, we also initialize and add the already allocated >> struct pending_irq to our radix tree, so that we can now easily find it >> by its virtual LPI number. >> We also read the property table to update the enabled bit and the >> priority for our new LPI, as we might have missed this during an earlier >> INVALL call (which only checks mapped LPIs). But we make sure that the >> property table is actually valid, as all redistributors might still >> be disabled at this point. >> Since write_itte_locked() now sees its first usage, we change the >> declaration to static. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-v3-its.c | 27 ++++++++ >> xen/arch/arm/vgic-v3-its.c | 138 >> ++++++++++++++++++++++++++++++++++++++- >> xen/include/asm-arm/gic_v3_its.h | 3 + >> 3 files changed, 165 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index 8864e0b..41fff64 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d, >> paddr_t vdoorbell_address, >> return 0; >> } >> +/* >> + * Connects the event ID for an already assigned device to the given >> VCPU/vLPI >> + * pair. The corresponding physical LPI is already mapped on the host >> side >> + * (when assigning the physical device to the guest), so we just >> connect the >> + * target VCPU/vLPI pair to that interrupt to inject it properly if >> it fires. >> + * Returns a pointer to the already allocated struct pending_irq that is >> + * meant to be used by that event. >> + */ >> +struct pending_irq *gicv3_assign_guest_event(struct domain *d, >> + paddr_t vdoorbell_address, >> + uint32_t vdevid, >> uint32_t eventid, >> + uint32_t virt_lpi) >> +{ >> + struct pending_irq *pirq; >> + uint32_t host_lpi = 0; > This should be INVALID_LPI and not 0. > > [...] > >> +/* >> + * For a given virtual LPI read the enabled bit and priority from the >> virtual >> + * property table and update the virtual IRQ's state in the given >> pending_irq. >> + * Must be called with the respective VGIC VCPU lock held. >> + */ >> +static int update_lpi_property(struct domain *d, struct pending_irq *p) >> +{ >> + paddr_t addr; >> + uint8_t property; >> + int ret; >> + >> + /* >> + * If no redistributor has its LPIs enabled yet, we can't access the >> + * property table. In this case we just can't update the properties, >> + * but this should not be an error from an ITS point of view. >> + */ >> + if ( !read_atomic(&d->arch.vgic.rdists_enabled) ) >> + return 0; > > AFAICT, there are no other place where the property would be updated. > Should we put a warning to let the user know that property will not be > updated? More that you don't return an error so no easy way to know > what's going on. So I thought about this again: If we handle an INV command while the respective redistributor has LPIs off, even hardware can't do anything, as having LPIs disabled means that there is no valid property table. So a hardware redistributor would probably just ignore this request. I see us only calling update_lpi_property during command handling, so I think it is safe to just silently ignore it, as hardware would do the same? >> + >> + addr = d->arch.vgic.rdist_propbase & GENMASK(51, 12); >> + >> + ret = vgic_access_guest_memory(d, addr + p->irq - LPI_OFFSET, >> + &property, sizeof(property), false); >> + if ( ret ) >> + return ret; >> + >> + write_atomic(&p->lpi_priority, property & LPI_PROP_PRIO_MASK); >> + >> + if ( property & LPI_PROP_ENABLED ) >> + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); >> + else >> + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); >> + >> + return 0; >> +} >> + >> /* Must be called with the ITS lock held. */ >> static int its_discard_event(struct virt_its *its, >> uint32_t vdevid, uint32_t vevid) >> @@ -538,6 +574,98 @@ static int its_handle_mapd(struct virt_its *its, >> uint64_t *cmdptr) >> return ret; >> } >> +static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr) >> +{ > > [...] > >> + /* >> + * radix_tree_insert() returns an error either due to an internal >> + * condition (like memory allocation failure) or because the LPI >> already >> + * existed in the tree. We don't support the latter case, so we >> always >> + * cleanup and return an error here in any case. >> + */ >> +out_remove_host_entry: >> + gicv3_remove_guest_event(its->d, its->doorbell_address, devid, >> eventid); > > Can gicv3_remove_guest_event fail? If yes, should we check the > return/add comment? If not, then we should have an ASSERT(....). Well, as we have an "if ( !ret ) return 0;" above, we only get here with ret being != 0, so this is in an error path already. I am not sure how we should react here then, I think reporting the first error is probably more meaningful. Cheers, Andre.
Hi, On 02/06/17 18:12, Julien Grall wrote: > Hi Andre, > > On 05/26/2017 06:35 PM, Andre Przywara wrote: >> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU >> pair and actually instantiates LPI interrupts. MAPI is just a variant >> of this comment, where the LPI ID is the same as the event ID. >> We connect the already allocated host LPI to this virtual LPI, so that >> any triggering LPI on the host can be quickly forwarded to a guest. >> Beside entering the domain and the virtual LPI number in the respective >> host LPI entry, we also initialize and add the already allocated >> struct pending_irq to our radix tree, so that we can now easily find it >> by its virtual LPI number. >> We also read the property table to update the enabled bit and the >> priority for our new LPI, as we might have missed this during an earlier >> INVALL call (which only checks mapped LPIs). But we make sure that the >> property table is actually valid, as all redistributors might still >> be disabled at this point. >> Since write_itte_locked() now sees its first usage, we change the >> declaration to static. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-v3-its.c | 27 ++++++++ >> xen/arch/arm/vgic-v3-its.c | 138 >> ++++++++++++++++++++++++++++++++++++++- >> xen/include/asm-arm/gic_v3_its.h | 3 + >> 3 files changed, 165 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index 8864e0b..41fff64 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d, >> paddr_t vdoorbell_address, >> return 0; >> } >> +/* >> + * Connects the event ID for an already assigned device to the given >> VCPU/vLPI >> + * pair. The corresponding physical LPI is already mapped on the host >> side >> + * (when assigning the physical device to the guest), so we just >> connect the >> + * target VCPU/vLPI pair to that interrupt to inject it properly if >> it fires. >> + * Returns a pointer to the already allocated struct pending_irq that is >> + * meant to be used by that event. >> + */ >> +struct pending_irq *gicv3_assign_guest_event(struct domain *d, >> + paddr_t vdoorbell_address, >> + uint32_t vdevid, >> uint32_t eventid, >> + uint32_t virt_lpi) >> +{ >> + struct pending_irq *pirq; >> + uint32_t host_lpi = 0; > This should be INVALID_LPI and not 0. > > [...] > >> +/* >> + * For a given virtual LPI read the enabled bit and priority from the >> virtual >> + * property table and update the virtual IRQ's state in the given >> pending_irq. >> + * Must be called with the respective VGIC VCPU lock held. >> + */ >> +static int update_lpi_property(struct domain *d, struct pending_irq *p) >> +{ >> + paddr_t addr; >> + uint8_t property; >> + int ret; >> + >> + /* >> + * If no redistributor has its LPIs enabled yet, we can't access the >> + * property table. In this case we just can't update the properties, >> + * but this should not be an error from an ITS point of view. >> + */ >> + if ( !read_atomic(&d->arch.vgic.rdists_enabled) ) >> + return 0; I was just looking at rdists_enabled, and think that using read_atomic() is a red herring. First rdists_enabled is a bool, so I have a hard time to imagine how it could be read non-atomically. I think the intention of making this read "somewhat special" was to cater for the fact that we write it under the domain lock, but read it here without taking it. But I think for this case we don't need any special read version, and anyway an *atomic* read would not help here. What we want is to make sure that rdist_propbase is valid before we see rdists_enabled gets true, this is what this check here is for. This should be solved by a write barrier between the two on the other side. Looking at Linux' memory_barriers.txt my understanding is that the matching barrier on the read side does not necessarily need to be an explicit barrier instruction, it could be a control flow dependency as well. And here we have that: we check rdists_enabled and bail out if it's not set, so neither the compiler nor the CPU can reorder this (as this would violate program semantics). Also rdists_enabled is a bit special in that it never gets reset once it became true, very much like the LPI enablement in the GICv3 spec. So I think we can really go with a normal read plus a comment. Does that make sense? Cheers, Andre.
On Fri, 9 Jun 2017, Andre Przywara wrote: > On 02/06/17 18:12, Julien Grall wrote: > > Hi Andre, > > > > On 05/26/2017 06:35 PM, Andre Przywara wrote: > >> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU > >> pair and actually instantiates LPI interrupts. MAPI is just a variant > >> of this comment, where the LPI ID is the same as the event ID. > >> We connect the already allocated host LPI to this virtual LPI, so that > >> any triggering LPI on the host can be quickly forwarded to a guest. > >> Beside entering the domain and the virtual LPI number in the respective > >> host LPI entry, we also initialize and add the already allocated > >> struct pending_irq to our radix tree, so that we can now easily find it > >> by its virtual LPI number. > >> We also read the property table to update the enabled bit and the > >> priority for our new LPI, as we might have missed this during an earlier > >> INVALL call (which only checks mapped LPIs). But we make sure that the > >> property table is actually valid, as all redistributors might still > >> be disabled at this point. > >> Since write_itte_locked() now sees its first usage, we change the > >> declaration to static. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> --- > >> xen/arch/arm/gic-v3-its.c | 27 ++++++++ > >> xen/arch/arm/vgic-v3-its.c | 138 > >> ++++++++++++++++++++++++++++++++++++++- > >> xen/include/asm-arm/gic_v3_its.h | 3 + > >> 3 files changed, 165 insertions(+), 3 deletions(-) > >> > >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > >> index 8864e0b..41fff64 100644 > >> --- a/xen/arch/arm/gic-v3-its.c > >> +++ b/xen/arch/arm/gic-v3-its.c > >> @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d, > >> paddr_t vdoorbell_address, > >> return 0; > >> } > >> +/* > >> + * Connects the event ID for an already assigned device to the given > >> VCPU/vLPI > >> + * pair. The corresponding physical LPI is already mapped on the host > >> side > >> + * (when assigning the physical device to the guest), so we just > >> connect the > >> + * target VCPU/vLPI pair to that interrupt to inject it properly if > >> it fires. > >> + * Returns a pointer to the already allocated struct pending_irq that is > >> + * meant to be used by that event. > >> + */ > >> +struct pending_irq *gicv3_assign_guest_event(struct domain *d, > >> + paddr_t vdoorbell_address, > >> + uint32_t vdevid, > >> uint32_t eventid, > >> + uint32_t virt_lpi) > >> +{ > >> + struct pending_irq *pirq; > >> + uint32_t host_lpi = 0; > > This should be INVALID_LPI and not 0. > > > > [...] > > > >> +/* > >> + * For a given virtual LPI read the enabled bit and priority from the > >> virtual > >> + * property table and update the virtual IRQ's state in the given > >> pending_irq. > >> + * Must be called with the respective VGIC VCPU lock held. > >> + */ > >> +static int update_lpi_property(struct domain *d, struct pending_irq *p) > >> +{ > >> + paddr_t addr; > >> + uint8_t property; > >> + int ret; > >> + > >> + /* > >> + * If no redistributor has its LPIs enabled yet, we can't access the > >> + * property table. In this case we just can't update the properties, > >> + * but this should not be an error from an ITS point of view. > >> + */ > >> + if ( !read_atomic(&d->arch.vgic.rdists_enabled) ) > >> + return 0; > > I was just looking at rdists_enabled, and think that using read_atomic() > is a red herring. > First rdists_enabled is a bool, so I have a hard time to imagine how it > could be read non-atomically. This is not a good argument, because if we want the read to be atomic, then we need to be using one of the _atomic functions regardless of the type. > I think the intention of making this read "somewhat special" was to > cater for the fact that we write it under the domain lock, but read it > here without taking it. I haven't looked at the specific of rdists_enabled in this implementaion, but be aware that in general writing a variable under a lock, and reading it atomically is not safe. You either read and write under a lock, or read and write atomically. > But I think for this case we don't need any > special read version, and anyway an *atomic* read would not help here. > > What we want is to make sure that rdist_propbase is valid before we see > rdists_enabled gets true, this is what this check here is for. This > should be solved by a write barrier between the two on the other side. > > Looking at Linux' memory_barriers.txt my understanding is that the > matching barrier on the read side does not necessarily need to be an > explicit barrier instruction, it could be a control flow dependency as > well. And here we have that: we check rdists_enabled and bail out if > it's not set, so neither the compiler nor the CPU can reorder this (as > this would violate program semantics). I think that this is true. > Also rdists_enabled is a bit special in that it never gets reset once it > became true, very much like the LPI enablement in the GICv3 spec. > > So I think we can really go with a normal read plus a comment. > > Does that make sense? The first motivation isn't right, but I think that the latter explanation makes sense.
Hi, On 09/06/17 20:14, Stefano Stabellini wrote: > On Fri, 9 Jun 2017, Andre Przywara wrote: >> On 02/06/17 18:12, Julien Grall wrote: >>> Hi Andre, >>> >>> On 05/26/2017 06:35 PM, Andre Przywara wrote: >>>> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU >>>> pair and actually instantiates LPI interrupts. MAPI is just a variant >>>> of this comment, where the LPI ID is the same as the event ID. >>>> We connect the already allocated host LPI to this virtual LPI, so that >>>> any triggering LPI on the host can be quickly forwarded to a guest. >>>> Beside entering the domain and the virtual LPI number in the respective >>>> host LPI entry, we also initialize and add the already allocated >>>> struct pending_irq to our radix tree, so that we can now easily find it >>>> by its virtual LPI number. >>>> We also read the property table to update the enabled bit and the >>>> priority for our new LPI, as we might have missed this during an earlier >>>> INVALL call (which only checks mapped LPIs). But we make sure that the >>>> property table is actually valid, as all redistributors might still >>>> be disabled at this point. >>>> Since write_itte_locked() now sees its first usage, we change the >>>> declaration to static. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> xen/arch/arm/gic-v3-its.c | 27 ++++++++ >>>> xen/arch/arm/vgic-v3-its.c | 138 >>>> ++++++++++++++++++++++++++++++++++++++- >>>> xen/include/asm-arm/gic_v3_its.h | 3 + >>>> 3 files changed, 165 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >>>> index 8864e0b..41fff64 100644 >>>> --- a/xen/arch/arm/gic-v3-its.c >>>> +++ b/xen/arch/arm/gic-v3-its.c >>>> @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d, >>>> paddr_t vdoorbell_address, >>>> return 0; >>>> } >>>> +/* >>>> + * Connects the event ID for an already assigned device to the given >>>> VCPU/vLPI >>>> + * pair. The corresponding physical LPI is already mapped on the host >>>> side >>>> + * (when assigning the physical device to the guest), so we just >>>> connect the >>>> + * target VCPU/vLPI pair to that interrupt to inject it properly if >>>> it fires. >>>> + * Returns a pointer to the already allocated struct pending_irq that is >>>> + * meant to be used by that event. >>>> + */ >>>> +struct pending_irq *gicv3_assign_guest_event(struct domain *d, >>>> + paddr_t vdoorbell_address, >>>> + uint32_t vdevid, >>>> uint32_t eventid, >>>> + uint32_t virt_lpi) >>>> +{ >>>> + struct pending_irq *pirq; >>>> + uint32_t host_lpi = 0; >>> This should be INVALID_LPI and not 0. >>> >>> [...] >>> >>>> +/* >>>> + * For a given virtual LPI read the enabled bit and priority from the >>>> virtual >>>> + * property table and update the virtual IRQ's state in the given >>>> pending_irq. >>>> + * Must be called with the respective VGIC VCPU lock held. >>>> + */ >>>> +static int update_lpi_property(struct domain *d, struct pending_irq *p) >>>> +{ >>>> + paddr_t addr; >>>> + uint8_t property; >>>> + int ret; >>>> + >>>> + /* >>>> + * If no redistributor has its LPIs enabled yet, we can't access the >>>> + * property table. In this case we just can't update the properties, >>>> + * but this should not be an error from an ITS point of view. >>>> + */ >>>> + if ( !read_atomic(&d->arch.vgic.rdists_enabled) ) >>>> + return 0; >> >> I was just looking at rdists_enabled, and think that using read_atomic() >> is a red herring. >> First rdists_enabled is a bool, so I have a hard time to imagine how it >> could be read non-atomically. > > This is not a good argument, because if we want the read to be atomic, > then we need to be using one of the _atomic functions regardless of the > type. OK, I see your point there. For the records (and to explain my ignorance ;-) I think it applies to a strict C standard point of view only. For all practical means I think a read into a variable of a native data type (especially that of a 1-byte sized bool) is always atomic on arm and arm64 - especially with the load/store architecture of ARM. Also I have a hard time to imagine intermediate values for a bool ;-), especially since rdist_enabled only goes from false to true once in a domain's lifetime. But nevertheless I can see and agree that to be C standard compliant we should use read_atomic(). Which makes me wonder if that would be true for other places in the code as well ... Another point of confusion may be that read_atomic() on its own does not seem to be enough here, since we also need the barrier mechanism, maybe even ACCESS_ONCE. But as I mentioned below this should be covered by the control flow guarantee is this case. I wonder if we should brainstorm if the usage of the atomic operations, the barriers and ACCESS_ONCE is really correct in the current Xen code. Comparing those to their Linux counterparts at least show some differences. >> I think the intention of making this read "somewhat special" was to >> cater for the fact that we write it under the domain lock, but read it >> here without taking it. > > I haven't looked at the specific of rdists_enabled in this > implementaion, but be aware that in general writing a variable under a > lock, and reading it atomically is not safe. You either read and write > under a lock, or read and write atomically. Agreed. rdists_enabled may be special here because it's a bool and only goes from false (initialized value) to true once (there is only one rdists_enabled assignment, which sets it to true). >> But I think for this case we don't need any >> special read version, and anyway an *atomic* read would not help here. >> >> What we want is to make sure that rdist_propbase is valid before we see >> rdists_enabled gets true, this is what this check here is for. This >> should be solved by a write barrier between the two on the other side. >> >> Looking at Linux' memory_barriers.txt my understanding is that the >> matching barrier on the read side does not necessarily need to be an >> explicit barrier instruction, it could be a control flow dependency as >> well. And here we have that: we check rdists_enabled and bail out if >> it's not set, so neither the compiler nor the CPU can reorder this (as >> this would violate program semantics). > > I think that this is true. > > >> Also rdists_enabled is a bit special in that it never gets reset once it >> became true, very much like the LPI enablement in the GICv3 spec. >> >> So I think we can really go with a normal read plus a comment. >> >> Does that make sense? > > The first motivation isn't right, but I think that the latter > explanation makes sense. OK, I think I agree with that. Cheers, Andre.
Hi Andre, On 07/06/17 18:49, Andre Przywara wrote: > On 02/06/17 18:12, Julien Grall wrote: >> On 05/26/2017 06:35 PM, Andre Przywara wrote: >>> + /* >>> + * radix_tree_insert() returns an error either due to an internal >>> + * condition (like memory allocation failure) or because the LPI >>> already >>> + * existed in the tree. We don't support the latter case, so we >>> always >>> + * cleanup and return an error here in any case. >>> + */ >>> +out_remove_host_entry: >>> + gicv3_remove_guest_event(its->d, its->doorbell_address, devid, >>> eventid); >> >> Can gicv3_remove_guest_event fail? If yes, should we check the >> return/add comment? If not, then we should have an ASSERT(....). > > Well, as we have an "if ( !ret ) return 0;" above, we only get here with > ret being != 0, so this is in an error path already. > I am not sure how we should react here then, I think reporting the first > error is probably more meaningful. Good point. I missed the if ( !ret ) return 0; in the code. Cheers,
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index 8864e0b..41fff64 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -876,6 +876,33 @@ int gicv3_remove_guest_event(struct domain *d, paddr_t vdoorbell_address, return 0; } +/* + * Connects the event ID for an already assigned device to the given VCPU/vLPI + * pair. The corresponding physical LPI is already mapped on the host side + * (when assigning the physical device to the guest), so we just connect the + * target VCPU/vLPI pair to that interrupt to inject it properly if it fires. + * Returns a pointer to the already allocated struct pending_irq that is + * meant to be used by that event. + */ +struct pending_irq *gicv3_assign_guest_event(struct domain *d, + paddr_t vdoorbell_address, + uint32_t vdevid, uint32_t eventid, + uint32_t virt_lpi) +{ + struct pending_irq *pirq; + uint32_t host_lpi = 0; + + pirq = get_event_pending_irq(d, vdoorbell_address, vdevid, eventid, + &host_lpi); + + if ( !pirq ) + return NULL; + + gicv3_lpi_update_host_entry(host_lpi, d->domain_id, virt_lpi); + + return pirq; +} + /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */ void gicv3_its_dt_init(const struct dt_device_node *node) { diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index e9b1cb4..c350fa5 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -270,9 +270,9 @@ static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid, * If vcpu_ptr is provided, returns the VCPU belonging to that collection. * Must be called with the ITS lock held. */ -bool write_itte_locked(struct virt_its *its, uint32_t devid, - uint32_t evid, uint32_t collid, uint32_t vlpi, - struct vcpu **vcpu_ptr) +static bool write_itte_locked(struct virt_its *its, uint32_t devid, + uint32_t evid, uint32_t collid, uint32_t vlpi, + struct vcpu **vcpu_ptr) { paddr_t addr; struct vits_itte itte; @@ -413,6 +413,42 @@ out_unlock: return ret; } +/* + * For a given virtual LPI read the enabled bit and priority from the virtual + * property table and update the virtual IRQ's state in the given pending_irq. + * Must be called with the respective VGIC VCPU lock held. + */ +static int update_lpi_property(struct domain *d, struct pending_irq *p) +{ + paddr_t addr; + uint8_t property; + int ret; + + /* + * If no redistributor has its LPIs enabled yet, we can't access the + * property table. In this case we just can't update the properties, + * but this should not be an error from an ITS point of view. + */ + if ( !read_atomic(&d->arch.vgic.rdists_enabled) ) + return 0; + + addr = d->arch.vgic.rdist_propbase & GENMASK(51, 12); + + ret = vgic_access_guest_memory(d, addr + p->irq - LPI_OFFSET, + &property, sizeof(property), false); + if ( ret ) + return ret; + + write_atomic(&p->lpi_priority, property & LPI_PROP_PRIO_MASK); + + if ( property & LPI_PROP_ENABLED ) + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); + else + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); + + return 0; +} + /* Must be called with the ITS lock held. */ static int its_discard_event(struct virt_its *its, uint32_t vdevid, uint32_t vevid) @@ -538,6 +574,98 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr) return ret; } +static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr) +{ + uint32_t devid = its_cmd_get_deviceid(cmdptr); + uint32_t eventid = its_cmd_get_id(cmdptr); + uint32_t intid = its_cmd_get_physical_id(cmdptr), _intid; + uint16_t collid = its_cmd_get_collection(cmdptr); + struct pending_irq *pirq; + struct vcpu *vcpu = NULL; + int ret = -1; + + if ( its_cmd_get_command(cmdptr) == GITS_CMD_MAPI ) + intid = eventid; + + spin_lock(&its->its_lock); + /* + * Check whether there is a valid existing mapping. If yes, behavior is + * unpredictable, we choose to ignore this command here. + * This makes sure we start with a pristine pending_irq below. + */ + if ( read_itte_locked(its, devid, eventid, &vcpu, &_intid) && + _intid != INVALID_LPI ) + { + spin_unlock(&its->its_lock); + return -1; + } + + /* Enter the mapping in our virtual ITS tables. */ + if ( !write_itte_locked(its, devid, eventid, collid, intid, &vcpu) ) + { + spin_unlock(&its->its_lock); + return -1; + } + + spin_unlock(&its->its_lock); + + /* + * Connect this virtual LPI to the corresponding host LPI, which is + * determined by the same device ID and event ID on the host side. + * This returns us the corresponding, still unused pending_irq. + */ + pirq = gicv3_assign_guest_event(its->d, its->doorbell_address, + devid, eventid, intid); + if ( !pirq ) + goto out_remove_mapping; + + vgic_init_pending_irq(pirq, intid); + + /* + * Now read the guest's property table to initialize our cached state. + * We don't need the VGIC VCPU lock here, because the pending_irq isn't + * in the radix tree yet. + */ + ret = update_lpi_property(its->d, pirq); + if ( ret ) + goto out_remove_host_entry; + + pirq->lpi_vcpu_id = vcpu->vcpu_id; + /* + * Mark this LPI as new, so any older (now unmapped) LPI in any LR + * can be easily recognised as such. + */ + set_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &pirq->status); + + /* + * Now insert the pending_irq into the domain's LPI tree, so that + * it becomes live. + */ + write_lock(&its->d->arch.vgic.pend_lpi_tree_lock); + ret = radix_tree_insert(&its->d->arch.vgic.pend_lpi_tree, intid, pirq); + write_unlock(&its->d->arch.vgic.pend_lpi_tree_lock); + + if ( !ret ) + return 0; + + /* + * radix_tree_insert() returns an error either due to an internal + * condition (like memory allocation failure) or because the LPI already + * existed in the tree. We don't support the latter case, so we always + * cleanup and return an error here in any case. + */ +out_remove_host_entry: + gicv3_remove_guest_event(its->d, its->doorbell_address, devid, eventid); + +out_remove_mapping: + spin_lock(&its->its_lock); + write_itte_locked(its, devid, eventid, + UNMAPPED_COLLECTION, INVALID_LPI, NULL); + 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)) @@ -579,6 +707,10 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its) case GITS_CMD_MAPD: ret = its_handle_mapd(its, command); break; + case GITS_CMD_MAPI: + case GITS_CMD_MAPTI: + ret = its_handle_mapti(its, command); + break; case GITS_CMD_SYNC: /* We handle ITS commands synchronously, so we ignore SYNC. */ break; diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index e78dadf..c6404ea 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_its.h @@ -175,6 +175,9 @@ struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, uint32_t veventid); int gicv3_remove_guest_event(struct domain *d, paddr_t vdoorbell_address, uint32_t vdevid, uint32_t veventid); +struct pending_irq *gicv3_assign_guest_event(struct domain *d, paddr_t doorbell, + uint32_t devid, uint32_t eventid, + uint32_t virt_lpi); void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id, uint32_t virt_lpi);
The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU pair and actually instantiates LPI interrupts. MAPI is just a variant of this comment, where the LPI ID is the same as the event ID. We connect the already allocated host LPI to this virtual LPI, so that any triggering LPI on the host can be quickly forwarded to a guest. Beside entering the domain and the virtual LPI number in the respective host LPI entry, we also initialize and add the already allocated struct pending_irq to our radix tree, so that we can now easily find it by its virtual LPI number. We also read the property table to update the enabled bit and the priority for our new LPI, as we might have missed this during an earlier INVALL call (which only checks mapped LPIs). But we make sure that the property table is actually valid, as all redistributors might still be disabled at this point. Since write_itte_locked() now sees its first usage, we change the declaration to static. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-v3-its.c | 27 ++++++++ xen/arch/arm/vgic-v3-its.c | 138 ++++++++++++++++++++++++++++++++++++++- xen/include/asm-arm/gic_v3_its.h | 3 + 3 files changed, 165 insertions(+), 3 deletions(-)