Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 24 Sep 2021 13:04:21 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@kernel.org>
To:     Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
cc:     Stefano Stabellini <sstabellini@kernel.org>,
        "jgross@suse.com" <jgross@suse.com>,
        "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "boris.ostrovsky@oracle.com" <boris.ostrovsky@oracle.com>,
        "julien@xen.org" <julien@xen.org>,
        "jbeulich@suse.com" <jbeulich@suse.com>,
        Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>
Subject: Re: [PATCH v3 2/2] xen-pciback: allow compiling on other archs than
 x86
In-Reply-To: <7310d23e-4193-3f4c-06da-606b30e73f24@epam.com>
Message-ID: <alpine.DEB.2.21.2109241258190.17979@sstabellini-ThinkPad-T480s>
References: <20210923095345.185489-1-andr2000@gmail.com> <20210923095345.185489-2-andr2000@gmail.com> <alpine.DEB.2.21.2109231252270.17979@sstabellini-ThinkPad-T480s> <f62a1e2c-4253-c998-c206-6bb0681a84fb@epam.com>
 <7310d23e-4193-3f4c-06da-606b30e73f24@epam.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323329-2142721594-1632513512=:17979"
Content-ID: <alpine.DEB.2.21.2109241258500.17979@sstabellini-ThinkPad-T480s>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-2142721594-1632513512=:17979
Content-Type: text/plain; CHARSET=UTF-8
Content-Transfer-Encoding: 8BIT
Content-ID: <alpine.DEB.2.21.2109241258501.17979@sstabellini-ThinkPad-T480s>

On Fri, 24 Sep 2021, Oleksandr Andrushchenko wrote:
> On 24.09.21 08:46, Oleksandr Andrushchenko wrote:
> > On 23.09.21 23:00, Stefano Stabellini wrote:
> >> On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>>
> >>> Xen-pciback driver was designed to be built for x86 only. But it
> >>> can also be used by other architectures, e.g. Arm.
> >>> Re-structure the driver in a way that it can be built for other
> >>> platforms as well.
> >>>
> >>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
> >> The patch looks good to me. Only one thing: on ARM32 I get:
> > WE do not yet support Xen PCI passthrough for ARM32

Keep in mind that it is possible to run ARM32 guests on an ARM64
hypervisor.


> >> drivers/xen/xen-pciback/conf_space_header.c: In function ‘bar_init’:
> >> drivers/xen/xen-pciback/conf_space_header.c:239:34: warning: right shift count >= width of type [-Wshift-count-overflow]
> >>       bar->val = res[pos - 1].start >> 32;
> >>                                     ^~
> >> drivers/xen/xen-pciback/conf_space_header.c:240:49: warning: right shift count >= width of type [-Wshift-count-overflow]
> >>       bar->len_val = -resource_size(&res[pos - 1]) >> 32;
> >>    
> >>    
> >> resource_size_t is defined as phys_addr_t and it can be 32bit on arm32.
> >>
> >>
> >> One fix is to surround:
> >>
> >> 		if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
> >> 			bar->val = res[pos - 1].start >> 32;
> >> 			bar->len_val = -resource_size(&res[pos - 1]) >> 32;
> >> 			return bar;
> >> 		}
> >>
> >> with #ifdef PHYS_ADDR_T_64BIT
> >>
> > This might not be correct. We are dealing here with a 64-bit BAR on a 32-bit OS.
> >
> > I think that this can still be valid use-case if BAR64.hi == 0. So, not sure
> >
> > we can just skip it with ifdef.
> >
> > Instead, to be on the safe side, we can have:
> >
> > config XEN_PCIDEV_STUB
> >          tristate "Xen PCI-device stub driver"
> >          depends on PCI && ARM64 && XEN
> > e.g. only allow building the "stub" for ARM64 for now.

This is a pretty drastic solution. I would be OK with it but I prefer
the solution below >> 16 >> 16.


> Or... there are couple of places in the kernel where PCI deals with the 32 bit shift as:
> 
> drivers/pci/setup-res.c:108:        new = region.start >> 16 >> 16;
> drivers/pci/iov.c:949:        new = region.start >> 16 >> 16;
> 
> commit cf7bee5a0bf270a4eace0be39329d6ac0136cc47
> Date:   Sun Aug 7 13:49:59 *2005* +0400
> 
> [snip]
> 
>      Also make sure to write high bits - use "x >> 16 >> 16" (rather than the
>      simpler ">> 32") to avoid warnings on 32-bit architectures where we're
>      not going to have any high bits.

I think this is the best option


> This might not be(?) immediately correct in case of LPAE though, e.g.
> 
> 64-bit BAR may tolerate 40-bit address in some use-cases?

It is correct for LPAE too, it is just that with LPAE it would be
unnecessary.

--8323329-2142721594-1632513512=:17979--