| Message ID | 20250325172727.600716-3-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
Hi Steward, On 25/03/2025 17:27, Stewart Hildebrand wrote: > We expect mmio read handlers to leave the bits above the access size > zeroed. Add an ASSERT to check this aspect of read handler behavior. > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> With one question below: Acked-by: Julien Grall <jgrall@amazon.com> > --- > v17->v18: > * no change > > v16->v17: > * new patch > > See https://lore.kernel.org/xen-devel/bc6660ef-59f1-4514-9792-067d987e3fbc@xen.org/ > > Also see 7db7bd0f319f ("arm/vpci: honor access size when returning an error") > > Also see xen/arch/arm/ioreq.c:handle_ioserv() > --- > xen/arch/arm/io.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index 653428e16c1f..68b5dca70026 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -37,6 +37,8 @@ static enum io_state handle_read(const struct mmio_handler *handler, > if ( !handler->ops->read(v, info, &r, handler->priv) ) > return IO_ABORT; > > + ASSERT((r & ~GENMASK_ULL((1U << info->dabt.size) * 8 - 1, 0)) == 0); OOI, I was expecing GENMASK to be sufficient because "r" is effectively an "unsigned long". So any reason to use GENMASK_ULL? Cheers,
On 3/30/25 18:08, Julien Grall wrote: > Hi Steward, > > On 25/03/2025 17:27, Stewart Hildebrand wrote: >> We expect mmio read handlers to leave the bits above the access size >> zeroed. Add an ASSERT to check this aspect of read handler behavior. >> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > > With one question below: > > Acked-by: Julien Grall <jgrall@amazon.com> Thanks! >> --- >> v17->v18: >> * no change >> >> v16->v17: >> * new patch >> >> See https://lore.kernel.org/xen-devel/bc6660ef-59f1-4514-9792-067d987e3fbc@xen.org/ >> >> Also see 7db7bd0f319f ("arm/vpci: honor access size when returning an error") >> >> Also see xen/arch/arm/ioreq.c:handle_ioserv() >> --- >> xen/arch/arm/io.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >> index 653428e16c1f..68b5dca70026 100644 >> --- a/xen/arch/arm/io.c >> +++ b/xen/arch/arm/io.c >> @@ -37,6 +37,8 @@ static enum io_state handle_read(const struct mmio_handler *handler, >> if ( !handler->ops->read(v, info, &r, handler->priv) ) >> return IO_ABORT; >> + ASSERT((r & ~GENMASK_ULL((1U << info->dabt.size) * 8 - 1, 0)) == 0); > > OOI, I was expecing GENMASK to be sufficient because "r" is effectively an "unsigned long". So any reason to use GENMASK_ULL? Only reason was that I took inspiration from Roger's suggestion at [1]. However, I agree that GENMASK is indeed sufficient. Feel free to fix up on commit. Lastly, this is OK to commit out of order as there is no dependency on the first patch. [1] https://lore.kernel.org/xen-devel/Zk72jPtd9iXhChbc@macbook/
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index 653428e16c1f..68b5dca70026 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -37,6 +37,8 @@ static enum io_state handle_read(const struct mmio_handler *handler, if ( !handler->ops->read(v, info, &r, handler->priv) ) return IO_ABORT; + ASSERT((r & ~GENMASK_ULL((1U << info->dabt.size) * 8 - 1, 0)) == 0); + r = sign_extend(dabt, r); set_user_reg(regs, dabt.reg, r);
We expect mmio read handlers to leave the bits above the access size zeroed. Add an ASSERT to check this aspect of read handler behavior. Suggested-by: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- v17->v18: * no change v16->v17: * new patch See https://lore.kernel.org/xen-devel/bc6660ef-59f1-4514-9792-067d987e3fbc@xen.org/ Also see 7db7bd0f319f ("arm/vpci: honor access size when returning an error") Also see xen/arch/arm/ioreq.c:handle_ioserv() --- xen/arch/arm/io.c | 2 ++ 1 file changed, 2 insertions(+)