[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.