| Message ID | 1506346478-1631-8-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Eric, On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote: > When the GITS_BASER<n>.Valid gets cleared, the data structures in > guest RAM are not provisionned anymore. The device, collection > and LPI lists stored in the in-kernel ITS represent the same > information in some form of cache. So let's void the cache. Just a thought. What about the opposite case, if the BASERs were previously not valid, and then become valid, is the ITS expected restore the state from memory? Thanks, -Christoffer
Hi Christoffer, On 16/10/2017 11:26, Christoffer Dall wrote: > Hi Eric, > > On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote: >> When the GITS_BASER<n>.Valid gets cleared, the data structures in >> guest RAM are not provisionned anymore. The device, collection >> and LPI lists stored in the in-kernel ITS represent the same >> information in some form of cache. So let's void the cache. > > Just a thought. What about the opposite case, if the BASERs were > previously not valid, and then become valid, is the ITS expected restore > the state from memory? > > Thanks, > -Christoffer > No the spec does not mandate that as far as I understand. Also the spec does not mandate clearing the cache when BASER moves to invalid (which this patch does), although this would have madee sense to me. So maybe we should drop that patch and only clean the cache when the IOCTL is used. Thanks Eric
On 16 October 2017 at 10:26, Christoffer Dall <cdall@linaro.org> wrote: > Hi Eric, > > On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote: >> When the GITS_BASER<n>.Valid gets cleared, the data structures in >> guest RAM are not provisionned anymore. The device, collection >> and LPI lists stored in the in-kernel ITS represent the same >> information in some form of cache. So let's void the cache. > > Just a thought. What about the opposite case, if the BASERs were > previously not valid, and then become valid, is the ITS expected restore > the state from memory? Architecturally speaking, it's a cache. As soon as the guest turns the GITS_CTLR.Enabled bit on, the ITS is permitted to start reading from the tables. It's an implementation choice whether it wants to preload a bunch of stuff, only load it up when it becomes necessary or even leave it all in memory and cache nothing. Eric wrote: > Also the spec does not mandate clearing the cache when BASER > moves to invalid (which this patch does), although this would > have made sense to me. The spec says that messing with GITS_BASER while GITS_CTRL.Enabled is set or GITS_CTLR.Quiescent is 0 is UNPREDICTABLE. And it says that once the OS has done the Enabled/Quiescent handshake then the ITS must have written out any dirty data to memory and stopped doing anything. So the effect is that BASER can't ever move to invalid while the caches are dirty. thanks -- PMM
On Mon, Oct 16, 2017 at 11:44:05AM +0200, Auger Eric wrote: > Hi Christoffer, > On 16/10/2017 11:26, Christoffer Dall wrote: > > Hi Eric, > > > > On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote: > >> When the GITS_BASER<n>.Valid gets cleared, the data structures in > >> guest RAM are not provisionned anymore. The device, collection > >> and LPI lists stored in the in-kernel ITS represent the same > >> information in some form of cache. So let's void the cache. > > > > Just a thought. What about the opposite case, if the BASERs were > > previously not valid, and then become valid, is the ITS expected restore > > the state from memory? > > > > Thanks, > > -Christoffer > > > No the spec does not mandate that as far as I understand. Also the spec > does not mandate clearing the cache when BASER moves to invalid (which > this patch does), although this would have madee sense to me. So maybe > we should drop that patch and only clean the cache when the IOCTL is used. > I thought Marc pointed out that there is at least one hardware implementation which does clean its cache when the BASER moves to invalid, so it's not unlikely that software could rely on this behavior, and I therefore think we should implement it, because it's not in violation of the spec either. Thanks, -Christoffer
On Mon, Oct 16, 2017 at 10:47:35AM +0100, Peter Maydell wrote: > On 16 October 2017 at 10:26, Christoffer Dall <cdall@linaro.org> wrote: > > Hi Eric, > > > > On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote: > >> When the GITS_BASER<n>.Valid gets cleared, the data structures in > >> guest RAM are not provisionned anymore. The device, collection > >> and LPI lists stored in the in-kernel ITS represent the same > >> information in some form of cache. So let's void the cache. > > > > Just a thought. What about the opposite case, if the BASERs were > > previously not valid, and then become valid, is the ITS expected restore > > the state from memory? > > Architecturally speaking, it's a cache. As soon as the guest > turns the GITS_CTLR.Enabled bit on, the ITS is permitted to > start reading from the tables. It's an implementation choice > whether it wants to preload a bunch of stuff, only load it > up when it becomes necessary or even leave it all in memory > and cache nothing. > > Eric wrote: > > Also the spec does not mandate clearing the cache when BASER > > moves to invalid (which this patch does), although this would > > have made sense to me. > > The spec says that messing with GITS_BASER while GITS_CTRL.Enabled > is set or GITS_CTLR.Quiescent is 0 is UNPREDICTABLE. And it > says that once the OS has done the Enabled/Quiescent handshake > then the ITS must have written out any dirty data to memory > and stopped doing anything. So the effect is that BASER > can't ever move to invalid while the caches are dirty. > Right, ok, so if we in fact clear the caches when the ITS is turned off and the BASER becomes invalid, and we load any data from memory as the ITS is turned on again (for the OS power management sequence) we should be good. And that means that software can't just 'move' a table location in memory by making the BASER invalid and then valid again, at least not without turning off the ITS. That makes sense to me, and so we don't need to do anything when the BASER becomes valid. Thanks, -Christoffer
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 3f4deb4..e280e62 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1471,8 +1471,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm, unsigned long val) { const struct vgic_its_abi *abi = vgic_its_get_abi(its); - u64 entry_size, device_type; + u64 entry_size; u64 reg, *regptr, clearbits = 0; + int type; /* When GITS_CTLR.Enable is 1, we ignore write accesses. */ if (its->enabled) @@ -1482,12 +1483,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm, case 0: regptr = &its->baser_device_table; entry_size = abi->dte_esz; - device_type = GITS_BASER_TYPE_DEVICE; + type = GITS_BASER_TYPE_DEVICE; break; case 1: regptr = &its->baser_coll_table; entry_size = abi->cte_esz; - device_type = GITS_BASER_TYPE_COLLECTION; + type = GITS_BASER_TYPE_COLLECTION; clearbits = GITS_BASER_INDIRECT; break; default: @@ -1499,10 +1500,24 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm, reg &= ~clearbits; reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT; - reg |= device_type << GITS_BASER_TYPE_SHIFT; + reg |= (u64)type << GITS_BASER_TYPE_SHIFT; reg = vgic_sanitise_its_baser(reg); *regptr = reg; + + if (reg & GITS_BASER_VALID) + return; + + switch (type) { + case GITS_BASER_TYPE_DEVICE: + vgic_its_free_device_list(kvm, its); + break; + case GITS_BASER_TYPE_COLLECTION: + vgic_its_free_collection_list(kvm, its); + break; + default: + break; + } } static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
When the GITS_BASER<n>.Valid gets cleared, the data structures in guest RAM are not provisionned anymore. The device, collection and LPI lists stored in the in-kernel ITS represent the same information in some form of cache. So let's void the cache. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)