On 14/03/2025 22:21, Andrew Cooper wrote:
>
>
> The bitmap_for_each() expression only inspects the bottom 8 bits of targets.
> Change it's type to uint8_t and use for_each_set_bit() which is more efficient
> over scalars.
>
> GICD_SGI_TARGET_LIST_MASK is 2 bits wide. Two cases discard the prior
> calculation, and one case exits early.
NIT: I'd add ... calculation of targets. I had to read it couple times to
understand.
>
> Therefore, move the GICD_SGI_TARGET_MASK calculation into the only case which
> wants it, and use MASK_EXTR() to simplify the expression.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <[email protected]>
> ---
> CC: Stefano Stabellini <[email protected]>
> CC: Julien Grall <[email protected]>
> CC: Volodymyr Babchuk <[email protected]>
> CC: Bertrand Marquis <[email protected]>
> CC: Michal Orzel <[email protected]>
>
> v2:
> * Split out of prior VGIC work as it's somewhat standalone.
> * Leave the case labels as they were.
> ---
> xen/arch/arm/vgic/vgic-mmio-v2.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c
> b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 670b335db2c3..da62a8078b5f 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -88,13 +88,12 @@ static void vgic_mmio_write_sgir(struct vcpu *source_vcpu,
> struct domain *d = source_vcpu->domain;
> unsigned int nr_vcpus = d->max_vcpus;
> unsigned int intid = val & GICD_SGI_INTID_MASK;
> - unsigned long targets = (val & GICD_SGI_TARGET_MASK) >>
> - GICD_SGI_TARGET_SHIFT;
> - unsigned int vcpu_id;
> + uint8_t targets = 0;
>
> switch ( val & GICD_SGI_TARGET_LIST_MASK )
> {
> case GICD_SGI_TARGET_LIST: /* as specified by targets
> */
> + targets = MASK_EXTR(val, GICD_SGI_TARGET_MASK);
> targets &= GENMASK(nr_vcpus - 1, 0); /* limit to existing VCPUs
> */
> break;
> case GICD_SGI_TARGET_OTHERS:
> @@ -104,11 +103,12 @@ static void vgic_mmio_write_sgir(struct vcpu
> *source_vcpu,
> case GICD_SGI_TARGET_SELF: /* this very vCPU only */
> targets = (1U << source_vcpu->vcpu_id);
> break;
> - case 0x3: /* reserved */
AFAICT 0x3 would not be even possible to trigger. It should have been 0x3<<24.
Reviewed-by: Michal Orzel <[email protected]>
~Michal
