| Message ID | 20210401085238.477270-2-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/ARM: Some vgic fixes and init sequence KVM selftests | expand |
Hi Eric, On Thu, 01 Apr 2021 09:52:31 +0100, Eric Auger <eric.auger@redhat.com> wrote: > > KVM_DEV_ARM_VGIC_GRP_ADDR group doc says we should return > -EEXIST in case the base address of the redist is already set. > We currently return -EINVAL. > > However we need to return -EINVAL in case a legacy REDIST address > is attempted to be set while REDIST_REGIONS were set. This case > is discriminated by looking at the count field. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v1 -> v2: > - simplify the check sequence > --- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > index 15a6c98ee92f0..013b737b658f8 100644 > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > @@ -791,10 +791,6 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, > size_t size = count * KVM_VGIC_V3_REDIST_SIZE; > int ret; > > - /* single rdist region already set ?*/ > - if (!count && !list_empty(rd_regions)) > - return -EINVAL; > - > /* cross the end of memory ? */ > if (base + size < base) > return -EINVAL; > @@ -805,11 +801,14 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, > } else { > rdreg = list_last_entry(rd_regions, > struct vgic_redist_region, list); > - if (index != rdreg->index + 1) > - return -EINVAL; > > - /* Cannot add an explicitly sized regions after legacy region */ > - if (!rdreg->count) > + if ((!count) != (!rdreg->count)) > + return -EINVAL; /* Mix REDIST and REDIST_REGION */ Urgh... The triple negation killed me. Can we come up with a more intuitive expression? Something like: /* Don't mix single region and discrete redist regions */ if (!count && rdreg->count) return -EINVAL; Does it capture what you want to express? Thanks, M.
Hi Marc, On 4/1/21 12:52 PM, Marc Zyngier wrote: > Hi Eric, > > On Thu, 01 Apr 2021 09:52:31 +0100, > Eric Auger <eric.auger@redhat.com> wrote: >> >> KVM_DEV_ARM_VGIC_GRP_ADDR group doc says we should return >> -EEXIST in case the base address of the redist is already set. >> We currently return -EINVAL. >> >> However we need to return -EINVAL in case a legacy REDIST address >> is attempted to be set while REDIST_REGIONS were set. This case >> is discriminated by looking at the count field. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v1 -> v2: >> - simplify the check sequence >> --- >> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c >> index 15a6c98ee92f0..013b737b658f8 100644 >> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c >> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c >> @@ -791,10 +791,6 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, >> size_t size = count * KVM_VGIC_V3_REDIST_SIZE; >> int ret; >> >> - /* single rdist region already set ?*/ >> - if (!count && !list_empty(rd_regions)) >> - return -EINVAL; >> - >> /* cross the end of memory ? */ >> if (base + size < base) >> return -EINVAL; >> @@ -805,11 +801,14 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, >> } else { >> rdreg = list_last_entry(rd_regions, >> struct vgic_redist_region, list); >> - if (index != rdreg->index + 1) >> - return -EINVAL; >> >> - /* Cannot add an explicitly sized regions after legacy region */ >> - if (!rdreg->count) >> + if ((!count) != (!rdreg->count)) >> + return -EINVAL; /* Mix REDIST and REDIST_REGION */ > > Urgh... The triple negation killed me. Can we come up with a more > intuitive expression? Something like: Yes sometimes I can be "different" ;-) > > /* Don't mix single region and discrete redist regions */ > if (!count && rdreg->count) > return -EINVAL;> > Does it capture what you want to express? yes it does! Thanks Eric > > Thanks, > > M. >
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c index 15a6c98ee92f0..013b737b658f8 100644 --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c @@ -791,10 +791,6 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, size_t size = count * KVM_VGIC_V3_REDIST_SIZE; int ret; - /* single rdist region already set ?*/ - if (!count && !list_empty(rd_regions)) - return -EINVAL; - /* cross the end of memory ? */ if (base + size < base) return -EINVAL; @@ -805,11 +801,14 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index, } else { rdreg = list_last_entry(rd_regions, struct vgic_redist_region, list); - if (index != rdreg->index + 1) - return -EINVAL; - /* Cannot add an explicitly sized regions after legacy region */ - if (!rdreg->count) + if ((!count) != (!rdreg->count)) + return -EINVAL; /* Mix REDIST and REDIST_REGION */ + + if (!count) + return -EEXIST; + + if (index != rdreg->index + 1) return -EINVAL; }
KVM_DEV_ARM_VGIC_GRP_ADDR group doc says we should return -EEXIST in case the base address of the redist is already set. We currently return -EINVAL. However we need to return -EINVAL in case a legacy REDIST address is attempted to be set while REDIST_REGIONS were set. This case is discriminated by looking at the count field. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v1 -> v2: - simplify the check sequence --- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)