| Message ID | 1508224209-15366-10-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger wrote: > When the GITS_BASER<n>.Valid gets cleared, the data structures in > guest RAM are not valid 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> > > --- > > v2 -> v3: > - add a comment and clear cache in if block > --- > virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index f3f0026f..084239c 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; > + > + /* Table no longer valid: clear cached data */ > + if (!(reg & GITS_BASER_VALID)) { > + 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; > + } > + } So we do this after setting the *regptr, which makes we worried about races. How are guest writes to these registers synchronized with, for example trying to save the tables. Perhaps we don't care because userspace should have stopped the VM before trying to save the ITS state? > } > > static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu, > -- > 2.5.5 > Otherwise looks good to me. -Christoffer
Hi Christoffer, On 18/10/2017 00:34, Christoffer Dall wrote: > On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger wrote: >> When the GITS_BASER<n>.Valid gets cleared, the data structures in >> guest RAM are not valid 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> >> >> --- >> >> v2 -> v3: >> - add a comment and clear cache in if block >> --- >> virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- >> 1 file changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index f3f0026f..084239c 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; >> + >> + /* Table no longer valid: clear cached data */ >> + if (!(reg & GITS_BASER_VALID)) { >> + 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; >> + } >> + } > > So we do this after setting the *regptr, which makes we worried about > races. > > How are guest writes to these registers synchronized with, for example > trying to save the tables. Perhaps we don't care because userspace > should have stopped the VM before trying to save the ITS state? Yes save & restore can happen only if all vcpus are locked. Same for user space accesses. Thanks Eric > > > >> } >> >> static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu, >> -- >> 2.5.5 >> > > Otherwise looks good to me. > -Christoffer >
On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote: > Hi Christoffer, > > On 18/10/2017 00:34, Christoffer Dall wrote: > > On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger wrote: > >> When the GITS_BASER<n>.Valid gets cleared, the data structures in > >> guest RAM are not valid 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> > >> > >> --- > >> > >> v2 -> v3: > >> - add a comment and clear cache in if block > >> --- > >> virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- > >> 1 file changed, 19 insertions(+), 4 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >> index f3f0026f..084239c 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; > >> + > >> + /* Table no longer valid: clear cached data */ > >> + if (!(reg & GITS_BASER_VALID)) { > >> + 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; > >> + } > >> + } > > > > So we do this after setting the *regptr, which makes we worried about > > races. > > > > How are guest writes to these registers synchronized with, for example > > trying to save the tables. Perhaps we don't care because userspace > > should have stopped the VM before trying to save the ITS state? > > Yes save & restore can happen only if all vcpus are locked. Same for > user space accesses. > So this cannot happens, because the save/restore operation will fail to get the lock of an executing VCPU which is modifying this function? Thanks, -Christoffer
Hi Christoffer, On 21/10/2017 16:31, Christoffer Dall wrote: > On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote: >> Hi Christoffer, >> >> On 18/10/2017 00:34, Christoffer Dall wrote: >>> On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger wrote: >>>> When the GITS_BASER<n>.Valid gets cleared, the data structures in >>>> guest RAM are not valid 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> >>>> >>>> --- >>>> >>>> v2 -> v3: >>>> - add a comment and clear cache in if block >>>> --- >>>> virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- >>>> 1 file changed, 19 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>>> index f3f0026f..084239c 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; >>>> + >>>> + /* Table no longer valid: clear cached data */ >>>> + if (!(reg & GITS_BASER_VALID)) { >>>> + 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; >>>> + } >>>> + } >>> >>> So we do this after setting the *regptr, which makes we worried about >>> races. >>> >>> How are guest writes to these registers synchronized with, for example >>> trying to save the tables. Perhaps we don't care because userspace >>> should have stopped the VM before trying to save the ITS state? >> >> Yes save & restore can happen only if all vcpus are locked. Same for >> user space accesses. >> > > So this cannot happens, because the save/restore operation will fail to > get the lock of an executing VCPU which is modifying this function? Yes that's my understanding. Thanks Eric > > Thanks, > -Christoffer >
On Sat, Oct 21, 2017 at 04:36:22PM +0200, Auger Eric wrote: > Hi Christoffer, > > On 21/10/2017 16:31, Christoffer Dall wrote: > > On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote: > >> Hi Christoffer, > >> > >> On 18/10/2017 00:34, Christoffer Dall wrote: > >>> On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger wrote: > >>>> When the GITS_BASER<n>.Valid gets cleared, the data structures in > >>>> guest RAM are not valid 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> > >>>> > >>>> --- > >>>> > >>>> v2 -> v3: > >>>> - add a comment and clear cache in if block > >>>> --- > >>>> virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- > >>>> 1 file changed, 19 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >>>> index f3f0026f..084239c 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; > >>>> + > >>>> + /* Table no longer valid: clear cached data */ > >>>> + if (!(reg & GITS_BASER_VALID)) { > >>>> + 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; > >>>> + } > >>>> + } > >>> > >>> So we do this after setting the *regptr, which makes we worried about > >>> races. > >>> > >>> How are guest writes to these registers synchronized with, for example > >>> trying to save the tables. Perhaps we don't care because userspace > >>> should have stopped the VM before trying to save the ITS state? > >> > >> Yes save & restore can happen only if all vcpus are locked. Same for > >> user space accesses. > >> > > > > So this cannot happens, because the save/restore operation will fail to > > get the lock of an executing VCPU which is modifying this function? > > Yes that's my understanding. > ok. If you respin, putting a comment here may be worth it. Thanks, -Christoffer
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index f3f0026f..084239c 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; + + /* Table no longer valid: clear cached data */ + if (!(reg & GITS_BASER_VALID)) { + 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 valid 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> --- v2 -> v3: - add a comment and clear cache in if block --- virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)