Re: [PATCH 4/4] ARM/vgic: Use for_each_set_bit() in vgic-mmio*

[Julien Grall <julien@xxxxxxx>]

Hi,

On 03/09/2024 14:19, Andrew Cooper wrote:
> On 03/09/2024 11:30 am, Michal Orzel wrote:
> > On 02/09/2024 12:03, Andrew Cooper wrote:
> > > These are all loops over a scalar value, and don't need to call general bitop
> > > helpers behind the scenes.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > ---
> > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > CC: Julien Grall <julien@xxxxxxx>
> > > CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > > CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > > CC: Michal Orzel <michal.orzel@xxxxxxx>
> > >
> > > Slighly RFC.  It's unclear whether len is the size of the register, or the
> > > size of the access.  For sub-GPR accesses, won't the upper bits be clear
> > > anyway?  If so, this can be simplified further.
> > See dispatch_mmio_write(). "len" refers to access size (i.e. 1/4/8 bytes). Each 
> > register is defined
> > with a region access i.e. VGIC_ACCESS_32bit that is compared with the actual 
> > access. In the current code
> > there is no register with 8B access. If there is a mismatch, there will be a 
> > data abort injected.
> > Also, the top 32-bits are not cleared anywhere, so I don't think we can drop 
> > masking. @Julien?
That's correct, there are no masking in the I/O dispatch helpers.

> Ok, so it is necessary right now to have the clamping logic in every
> callback.
>
> However, given that the size is validated before dispatching, clamping
> once in dispatch_mmio_write() would save a lot of repeated code in the
> callbacks.
>
> i.e., I think this:
>
> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> index bd4caf7fc326..e8b9805a0b2c 100644
> --- a/xen/arch/arm/vgic/vgic-mmio.c
> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> @@ -590,6 +590,9 @@ static int dispatch_mmio_write(struct vcpu *vcpu,
> mmio_info_t *info,
>       if ( !region )
>           return 0;
>   
> +    if ( len < sizeof(data) )
> +        data &= ~((1UL << (len * 8)) - 1);
> +
I think it would make sense to move the masking one level higher in 
handle_write() (arch/arm/io.c). So this would cover all the emulation 
helpers.
Cheers,

--
Julien Grall