From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access Date: Fri, 25 Sep 2015 17:44:45 +0100 Message-ID: <1443199485.25250.212.camel@citrix.com> References: <1443192698-16163-1-git-send-email-julien.grall@citrix.com> <1443192698-16163-4-git-send-email-julien.grall@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZfW77-0000oJ-BH for xen-devel@lists.xenproject.org; Fri, 25 Sep 2015 16:44:49 +0000 In-Reply-To: <1443192698-16163-4-git-send-email-julien.grall@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , xen-devel@lists.xenproject.org Cc: Vijaya.Kumar@caviumnetworks.com, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com, vijay.kilari@gmail.com List-Id: xen-devel@lists.xenproject.org On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote: > The guest may try to load data from the emulated MMIO region using > instruction with Sign-Extension (i.e ldrs*). This can happen for any > access smaller than the register size (byte/half-word for aarch32, > byte/half-word/word for aarch64). > > The support of sign-extension was limited for byte access in vGIG "vGIC" > emulation. Although there is no reason to not have it generically. > > So move the support just after we get the data from the MMIO emulation. > > Signed-off-by: Julien Grall > > --- > > I was thinking to completely drop the sign-extension support in Xen as > it will be very unlikely to use ldrs* instruction to access MMIO. > Although the code is fairly small, so it doesn't harm to keep it > generically. Yes, I think we should keep it, since we don't control what instructions are used to access MMIO, however unlikely... > Changes in v2: > - Patch added > --- > xen/arch/arm/io.c | 29 ++++++++++++++++++++++++++++- > xen/arch/arm/vgic-v2.c | 10 +++++----- > xen/arch/arm/vgic-v3.c | 4 ++-- > xen/include/asm-arm/vgic.h | 8 +++----- > 4 files changed, 38 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index 32b2194..e1b03a2 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -23,6 +23,32 @@ > #include > #include > > +static int handle_read(mmio_read_t read_cb, struct vcpu *v, > + mmio_info_t *info, register_t *r) > +{ > + uint8_t size = (1 << info->dabt.size) * 8; > + > + if ( !read_cb(v, info, r) ) > + return 0; > + > + /* > + * Extend the bit sign if required. I think you meant s/bit sign/sign bit/ but more correct would be "Sign extend if required". > + * Note that we expect the read handler to have zeroed the bit > + * unused in the register. "... to have zeroed the unused bits in the register". But I think "unused" is a bit misleading, you mean the ones outside the requested access size, those bits are still "used" IYSWIM. I can't think of a terse term for "outside the requested access size I'm afraid. Did you confirm that all existing handlers meet this requirement? Perhaps an ASSERT would be handy? > + */ > + if ( info->dabt.sign && (*r & (1UL << (size - 1)) )) > + { > + /* > + * We are relying on register_t as the same size as > + * an unsigned long or order to keep the 32bit some smaller "order"? I'm not sure what you meant here so I can't suggest an alternative. > + */ > + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); > + *r |= (~0UL) << size; I think here and in the initial if you need to be careful of the case where size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size of the variable is, I think, undefined behaviour. It's also a waste of time sign extending in that case. Ian.