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

Re: [PATCH] x86/passthrough: Fix hvm_gsi_eoi() build with GCC 12


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 28 Oct 2021 15:26:41 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=euJgYPFU8bUrNLRObsngksn9vuyg3OJUWI2FfPHJMwQ=; b=NTuSHkMCTqAVg+sKpEpynsZ1xvSDWrfuaiEuUMJXaKlL0lqAoyDzb8qJNBv5PbfJOQMN7ZT3XRjSTaTumHVwy7AeEuzL7aWN0rBn1zIpCY/STKQGxN7SMIkbXMEdrVNfGdtjIAv5AN1RVzrQOPT9FyVEbD/s98Jwl+p5CmD6Ml2MGCM0/iNejOqM3TD0e/E7tst/unWsa5vuaC06lG5ncEQxmZhcnGJkU8VQ/Nd7pbUPEGlfieCHsyDuOMajniv5EMR+qPK8EP+T2vatzZYZHo1PwbvCpB1ux8djW/fFmApuNRNhZKbzjhbkn/imaldh7Lv2srvF5cES2wePKUTbYw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JjRuQMBujP9/VOMFQqKmEZkHxLMiQ/xU5WhTLnScaQIQzg8apCA8b5eUENBDnZVaq0rlVOTRHrKN6/gxzFnKfXny4xFpaEGYiMxx6u+t3oz/0Sn/kYBbnfbqGGSLGTOp8C/1DEpazVA2eZE1WtkEIeKsaP97VnvPFn7J+V74TP38LyYc7TLEaxOCFoWr0fzSANfUmUL/oAAnTirNfI0LGVBrzSe7KsSvaxMbHIdN410NtnfxtAqDerOXrnhVBSs6VpOHSmIUUV/b7xgX0Ch+eohSTp+Zfl/PbCCY1jouuingHnfiYaO1nTysYKu8PUlE2kCIrCUkcoPyy/uomT3/9A==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Thu, 28 Oct 2021 13:26:58 +0000
  • Ironport-data: A9a23:ujx4Sq04+yR8IJt33fbD5SV2kn2cJEfYwER7XKvMYLTBsI5bpz0Dm zROC22Caf3ZZTT1fdskOY/gpxkGv8LXzodhTwBkpC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkS5PE3oHJ9RGQ74nRLlbHILOCan8ZqTNMEn970Es7wb5h2+aEvPDia++zk YKqyyHgEAfNNw5cagr4PIra9XuDFNyr0N8plgRWicJj5TcypFFMZH4rHomjLmOQf2VhNrXSq 9Avbl2O1jixEx8FUrtJm1tgG6EAaua60QOm0hK6V0U+6/TrS+NbPqsTbZIhhUlrZzqhjtVql ogSlqGKQ0QAAvTMwtQ+eQNmOnQrVUFG0OevzXmXtMWSywvNcmf2wuUoB0YzVWEa0r8pWycUr 6VecW1TKEDY7w616OvTpu1EnMMsIdOtJIoCknph0SvYHbAtRpWrr6DiuIEFgGZr2psm8fD2P u8jSmIzXj/5XD5gZVgyJrU6l7ehiSyqG9FfgA3M/vdmi4TJ9yRu1JD9PdyTfcaFLe1UgUSwt m/A537+ABwRKJqY0zXt2mKhgKrDkD32XKoWFaak7bh6jVuL3GsRBRYKE1yhrpGRqGSzRtZeI Ew84Tc1oO4580nDczXmd0Tm+jje5EdaAocOVb1hgO2Q9kbKyxecHTZZER18UtZlnfNvRWYa9 XWTofq8UFSDr4apYX6a876Vqxa7Ni4UMXIOaEc4cOcV3zXwiNps1kyXH76PBIbw14evQWihn FhmuQBn3+1L5fPnwZlX6rwub9iEnZPOUhIurjveWmao/2uVj6b0OtT2tzA3ARtGRbt1r2VtX lBYxKByD8hUVPlhcRBhps1WRdlFAN7eaVXhbaZHRcVJythU0yfLkXpsyD9/Plx1Fc0PZCXkZ kTe0SsIusQOZyXyN/QpMtLtYyjP8UQGPY+8PhwzRoEXCqWdiSfdpH0+DaJu9zm1+KTTrU3PE cjCKpv9ZZrrIa9m0CC3V48gPUwDnUgDKZfobcmjlXyPiOPGDFbMEOttGAbeP4gRsfLfyC2Io ok3Cid/40gGOAEISnKMqtB7wJFjBSVTOK0aXOQMJ7PdeFI8QT1xYxITqJt4E7FYc21uvr6g1 lm2W1NCyUq5gnvCKA6QbWtkZq+pVpF6xU/X9wR3Vbpx83R8M4up8okFcJ47Iesu+OB5lKYmR PgZYcSQRP9IT22fqTgaaJD8qq1kdQiq2l3Sb3b0PmBncs4yXRHN9//lYhDrqHsEAB2ouJZsu LanzA7aH8YOHlwwEMbMZfuz5FqtpnxByvlqVk7FL4ALKkXh+YRnMQLrifozL51eIBnP3GLCh Q2XHQ0Zta/GpIpsqIvFgqWNroGIFepiHxUFQzmHvOjubSSDpzit245NVuqMbAvxbmKs9fXwf /hRwtH9LOYDwARAvb1jHus51qk5/dbu+eNXl1w2AHXRYl23Ibp8OX3aj9JXv6hAy7IF6wu7X kWDpotTNbmTYZ63FVcQIEwub/iZ1OFSkT7XtKxnLEL/7S5x3byGTUQNYEXc1H0DdON4YNE/3 OMsmM8K8Aju2BMlP+GPgj1Q62nRfGcLVL8qt81CDYLm4ubxJoquvXAI5vfK3ayy
  • Ironport-hdrordr: A9a23:HzRHEqpiT42nwMvMUSLTpvQaV5uxL9V00zEX/kB9WHVpm5Oj+P xGzc526farslsssREb+OxpOMG7MBThHLpOkPMs1NCZLXTbUQqTXfpfBO7ZrQEIdBeOlNK1uZ 0QFpSWTeeAcWSS7vyKkTVQcexQueVvmZrA7Yy1rwYPPHFXguNbnn9E426gYzNLrWJ9dPwE/f Snl656T23KQwVpUi33PAhOY8Hz4/nw0L72ax8PABAqrCGIkDOT8bb/VzyVxA0XXT9jyaortT GtqX202oyT99WAjjPM3W7a6Jpb3PPn19t4HcSJzuwYMC/lhAqEbJloH5eCoDc2iuey70tCqq iAnz4Qe+BIr1/BdGC8phXgnyHmzTYV8nfnjWSVhHPyyPaJDQ4SOo5kv8Z0YxHZ400vsJVXy6 RQxV+UsJJREFfpgDn9z8KgbWAqqmOE5V4Z1cIDhX1WVoUTLJVLq5YEwU9TGJAcWArn9YEcFv V0Bs203ocZTbqjVQGbgoBT+q3vYpxqdS32B3Tq+/blnAS+pUoJj3fxn6ck7zM9HJFUcegz2w 2LCNUuqFh0dL5lUUtKPpZ3fSKGMB2/ffvyChPmHb3GLtBOB5ufke+93F0KjNvaDKDgiqFC3q j8bA==
  • Ironport-sdr: KqBACIgmDRhH9xszq+YtsbBZKhzFHjIusonzHXOGuChy/DjARYHFwQQpfAQZxsOgApMS0DhiVi wp/P/Ro3lY9uaS+njPKCmAOGq15t95XfcvAA73ykYX6TaFbp9wZsqH6zQij/KlcfPeI3wXaWuk ViHAVMZcEh2t397sSBLSfbIOy6g1cenmiCMc24/d8PAWl4CbqZhDk4IJG8ErtoD9HRfT2ul8r4 dbKyt326qjNFdHhdUCi8uBzAtqWGr+GrRgzi8LCl9kCIm24+woIzasl7wJF/EJ3dIhN1TPh7pB Jk2BLyOIjeE8JOdqgqmg644s
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Oct 28, 2021 at 01:15:13PM +0100, Andrew Cooper wrote:
> On 28/10/2021 08:31, Roger Pau Monné wrote:
> > On Wed, Oct 27, 2021 at 09:07:13PM +0100, Andrew Cooper wrote:
> >> GCC master (nearly version 12) complains:
> >>
> >>   hvm.c: In function 'hvm_gsi_eoi':
> >>   hvm.c:905:10: error: the comparison will always evaluate as 'true' for 
> >> the
> >>   address of 'dpci' will never be NULL [-Werror=address]
> >>     905 |     if ( !pirq_dpci(pirq) )
> >>         |          ^
> >>   In file included from /local/xen.git/xen/include/xen/irq.h:73,
> >>                    from /local/xen.git/xen/include/xen/pci.h:13,
> >>                    from /local/xen.git/xen/include/asm/hvm/io.h:22,
> >>                    from /local/xen.git/xen/include/asm/hvm/domain.h:27,
> >>                    from /local/xen.git/xen/include/asm/domain.h:7,
> >>                    from /local/xen.git/xen/include/xen/domain.h:8,
> >>                    from /local/xen.git/xen/include/xen/sched.h:11,
> >>                    from /local/xen.git/xen/include/xen/event.h:12,
> >>                    from hvm.c:20:
> >>   /local/xen.git/xen/include/asm/irq.h:140:34: note: 'dpci' declared here
> >>     140 |             struct hvm_pirq_dpci dpci;
> >>         |                                  ^~~~
> >>
> >> The location marker is unhelpfully positioned and upstream may get around 
> >> to
> >> fixing it.  The complaint is intended to be:
> >>
> >>   if ( !((pirq) ? &(pirq)->arch.hvm.dpci : NULL) )
> >>                   ^~~~~~~~~~~~~~~~~~~~~~
> >>
> >> which is a hint that the code is should be simplified to just:
> >>
> >>   if ( !pirq )
> > I likely need more coffee, but doesn't this exploit how the macro
> > (pirq_dpci) is currently coded?
> 
> The way pirq_dpci() is currently coded, this is nonsense, which GCC is
> now highlighting.
> 
> It would be a very different thing if the logic said:
> 
> struct pirq *pirq = pirq_info(d, gsi);
> struct hvm_pirq_dpci *dpci = pirq_dpci(pirq);
> 
> /* Check if GSI is actually mapped. */
> if ( !dpci )
>     return;
> 
> but it doesn't. Getting a non-null pirq pointer from pirq_info(d, gsi)
> does identify that the GSI is mapped, and the dpci nested structure is
> not used in this function.  I would expect this property to remain true
> even if we alter the data layout.

I think we have further assertions that this will be true:

 1. d can only be an HVM domain given the callers of the function.
 2. The pirq struct is obtained from the list of pirqs owned by d.

In which case it's assured that the pirq will always have a dpci. I
think it might be better if the check was replaced with:

if ( !is_hvm_domain(d) || !pirq )
{
    ASSERT(is_hvm_domain(d));
    return;
}

Here Xen cares that pirq is not NULL and that d is an HVM domain
because hvm_gsi_deassert will assume so.

Thanks, Roger.



 


Rackspace

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