* RE: [EXTERNAL] Re: [RFC PATCH 0/4] DirectX on Linux
From: Steve Pronovost @ 2020-05-20 15:34 UTC (permalink / raw)
To: Daniel Vetter
Cc: Dave Airlie, Sasha Levin, linux-hyperv@vger.kernel.org,
Stephen Hemminger, Ursulin, Tvrtko, Greg Kroah-Hartman,
Haiyang Zhang, LKML, dri-devel, Chris Wilson,
Linux Fbdev development list, Iouri Tarassov, Deucher, Alexander,
KY Srinivasan, Wei Liu, Hawking Zhang, Max McMullen,
Jesse Natalie, Shawn Hargreaves
In-Reply-To: <MWHPR21MB02870909F08EBA08EB903635C7B60@MWHPR21MB0287.namprd21.prod.outlook.com>
[resending as plain text, sorry about that]
Thanks Daniel, more below.
From: Daniel Vetter <mailto:daniel@ffwll.ch>
Sent: Wednesday, May 20, 2020 12:41 AM
To: Steve Pronovost <mailto:spronovo@microsoft.com>
Cc: Dave Airlie <mailto:airlied@gmail.com>; Sasha Levin <mailto:sashal@kernel.org>; mailto:linux-hyperv@vger.kernel.org; Stephen Hemminger <mailto:sthemmin@microsoft.com>; Ursulin, Tvrtko <mailto:tvrtko.ursulin@intel.com>; Greg Kroah-Hartman <mailto:gregkh@linuxfoundation.org>; Haiyang Zhang <mailto:haiyangz@microsoft.com>; LKML <mailto:linux-kernel@vger.kernel.org>; dri-devel <mailto:dri-devel@lists.freedesktop.org>; Chris Wilson <mailto:chris@chris-wilson.co.uk>; Linux Fbdev development list <mailto:linux-fbdev@vger.kernel.org>; Iouri Tarassov <mailto:iourit@microsoft.com>; Deucher, Alexander <mailto:alexander.deucher@amd.com>; KY Srinivasan <mailto:kys@microsoft.com>; Wei Liu <mailto:wei.liu@kernel.org>; Hawking Zhang <mailto:Hawking.Zhang@amd.com>
Subject: Re: [EXTERNAL] Re: [RFC PATCH 0/4] DirectX on Linux
Hi Steve,
Sounds all good, some more comments and details below.
On Wed, May 20, 2020 at 5:47 AM Steve Pronovost <mailto:spronovo@microsoft.com> wrote:
Hey guys,
Thanks for the discussion. I may not be able to immediately answer all of your questions, but I'll do my best 😊.
drivers/hyperv sounds like it could be a better location. We weren't too sure where to put this, we though /drivers/gpu would be appropriate given this deal with GPUs, but I get your point... this is a vGPU driver that really only works when being run under Hyper-V, so drivers/hyperv is likely more appropriate.
I think "it's a virtual gpu" is the wrong sales pitch, as is "only runs on $platform". We have lots of drm drivers in drivers/gpu that fit that bill. The better pitch I think is "it's a not a gpu, it's a dx12 protocol pipe" and "we actually do not want to integrate with the linux gpu ecosystem and primitives, we want to integrate with dx12 ecosystem and primitives to make the seamless rdp/rail/vail stuff work nicely". Below some more thoughts on the technical said.
[spronovo] Agreed. As I mentioned in another reply, that protocol isn’t tied to DX… but the point you are making is still valid. This is really a projection of the Windows native abstraction of a GPU that windows user mode driver (dx, gl, cl, vulkan, cuda, etc…) are familiar with and use to communicate with the GPU… This effectively enable porting of these user mode driver to Linux inside of WSL and allow them to share the GPU with the host. Our goal is to offer CL/EGL/GLX/CUDA/… API support for applications running inside of WSL and integrate their output on the Windows desktop through the Wayland compositor we are building. The fact that we are using layer to implement some of these APIs (to reduce our partners work among other thing) is just an implementation details that most application shouldn’t have to worry about… “it just works” 😊. From that perspective we’re fine moving the driver under a different node than /driver/gpu 😊.
In term of presentation, I need to clarify a few things. We announced today that we're also adding support for Linux GUI applications. The way this will work is roughly as follow. We're writing a Wayland compositor that will essentially bridge over RDP-RAIL (RAIL=Remote Application Integrated Locally). We're starting from a Weston base. Weston already has an RDP Backend, but that's for a full desktop remoting scheme. Weston draws a desktop and remote it over RDP... and then you can peek at that desktop using an rdp client on the Windows side. RAIL works differently. In that case our wayland compositor no longer paint a desktop... instead it simply forward individual visual / wl_surface over the RDP RAIL channel such that these visual can be displayed on the Windows desktop. The RDP client create proxy window for each of these top level visual and their content is filled with the data coming over the RDP channel. All pixels are owned by the RDP server/WSL... so these windows looks different than native window are they are painted and themed by WSL. The proxy window on the host gather input and inject back over RDP... This is essentially how application remoting works on windows and this is all publicly documented as part of the various RDP protocol specification. As a matter of fact, for the RDP server on the Weston side we are looking at continue to leverage FreeRDP (and provide fixes/enhancement as needed to the public project). Further, we're looking at further improvement down this path to avoid having to copy the content over the RAIL channel and instead just share/swap buffer between the guest and the host. We have extension to the RDP protocol, called VAIL (Virtualized Application Integrated Locally) which does that today. Today this is only use in Windows on Windows for very specific scenario. We're looking at extending the public RDP protocol with these VAIL extension to make this an official Microsoft supported protocol which would allow us to target this in WSL. We have finished designing this part in details. Our goal would be to leverage something along the line of wl_drm, dma-buf, dma-fence, etc... This compositor and all our contribution to FreeRDP will be fully open source, including our design doc. We're not quite sure yet whether this will be offered as a separate project entirely distinct from it's Weston root... or if we'll propose an extension to Weston to operate in this mode. We would like to build it such that in theory any Wayland compositor could add support for this mode of operation if they want to remote application to a Windows host (over the network, or on the same box).
Sounds like a solid plan for presentation. I think this is all up to wayland/weston folks to figure out with you, from the kernel side I have only one concern (and I discussed that with a few folks already on irc, I think they're at least on this thread involved within microsoft too in some form): If we do integrate with linux concepts like wl_drm/dma-buf/fence and so on then we end up with a normal gpu driver, with with lots of blobby components all around that can't be opened (since large chunks written by hw vendors, so nothing microsoft can do about them). That's the awkward exception (why microsoft but not other gpu hw vendors/plaforms/whatever?) that we need to avoid.
But wayland doesn't really need dma-buf and the wl_drm protocols afaiui, as long as the egl extensions work you can have whatever private wayland protocol in your winsys code you want to shovel the buffers and syncobj from client to the wayland-rdp-rail compositor. If that uses dx12 native handles for these things we side-step the awkward exception question for linux gpu stack since it all stays 100% contained in drivers/hv. Just try to avoid the nvidia fail of insisting that you need your own set of egl extensions (egl_streams and egl_drm_kms_reinvented_but_badly and a few others iirc) for everything, that makes it very painful for all the compositor projects since they need duplicated code for no good reason :-)
[spronovo] Agreed. I haven’t fully dig into the details for the VAIL enhancement to our Wayland compositor that enables buffer sharing/swapping. This is something we do on window and are quite familiar with the mechanic/concept around this… but new to doing this under Wayland. I’m sharing early thinking so apology if I get some stuff wrong 😊… I fundamentally agree with the spirit of what you are saying. Fundamentally our goal here is that we want to avoid having Wayland client do anything non-standard to interop with our projected graphics stack. We want Wayland client to work unmodified. It’s not entirely clear to me at the moment whether this buffer handshake is entirely hidden inside of Mesa (where we could do whatever we want as you say, and just reuse what we already have) or visible to some Wayland client, in which case we would want to ensure we present a public interface that is compatible. I gather from your suggestion that it is the former. In a nutshell we want to be as standard as possible so things just work and avoid having to chase a never ending set of app compat problem 😊.
Also looking at the fun the virtio folks have right now trying to get virgl dma-buf handles in the guest shared with other virtio devices or virgl instance in some other guest and getting it working ... If you avoid the dma-buf integration that might also sidestep a lot of technical headaches and be the simpler solution. At least as long as we're talking about wsl2 only.
[spronovo] Yeah, will definitely look into this.
We see /dev/dxg really as a projection of the GPU when running in WSL such that the GPU can be shared between WSL and the host... not something that would coexist "at the same time" with a real DRM GPU.
We have consider the possibility of bringing DX to Linux with no Windows cord attached. I'm not ready to discuss this at this time 😊... but in the hypothetical that we were do this, DX would be running on top of DRI/DRM on native Linux. We likely would be contributing some changes to DRM to address area of divergence and get better mapping for our user mode driver, but we wouldn't try to shoehorn /dev/dxg into the picture. In that hypothetical world, we would essentially have DX target DRM on native Linux and DX continue to target DXG in WSL to share the GPU with the host. I think this further reinforce the point you guys were making that the right place for our current dxgkrnl driver to live in would be /drivers/hyperv/dxgkrnl. In insight, I totally agree 😊.
We had a pile of discussions on irc about some of the ideas floating around for extending drm with some of the newer memory/sync concepts. So very much interested, but that's indeed a different fish and most likely will look a lot different to the dxgkrnl interface, while ofc still aiming to give the same power to apps. At least that's the goal, but aside from some prototype in some place this really doesn't exist yet anywhere.
[spronovo] There are indeed some very interesting and hard challenges if we wanted to do this 😊. But yeah, this would be totally separate from the dxgkrnl interface. Dxgkrnl is really meant for WSL (or Linux in a Windows hosted VM)... which we should have made clearer and avoid some of the confusion.
I think this cover all questions, let me know if I missed anything.
I think it's all good.
Cheers, Daniel
Thanks,
Steve
-----Original Message-----
From: Daniel Vetter <mailto:daniel@ffwll.ch>
Sent: Tuesday, May 19, 2020 4:01 PM
To: Dave Airlie <mailto:airlied@gmail.com>
Cc: Sasha Levin <mailto:sashal@kernel.org>; mailto:linux-hyperv@vger.kernel.org; Stephen Hemminger <mailto:sthemmin@microsoft.com>; Ursulin, Tvrtko <mailto:tvrtko.ursulin@intel.com>; Greg Kroah-Hartman <mailto:gregkh@linuxfoundation.org>; Haiyang Zhang <mailto:haiyangz@microsoft.com>; LKML <mailto:linux-kernel@vger.kernel.org>; dri-devel <mailto:dri-devel@lists.freedesktop.org>; Chris Wilson <mailto:chris@chris-wilson.co.uk>; Steve Pronovost <mailto:spronovo@microsoft.com>; Linux Fbdev development list <mailto:linux-fbdev@vger.kernel.org>; Iouri Tarassov <mailto:iourit@microsoft.com>; Deucher, Alexander <mailto:alexander.deucher@amd.com>; KY Srinivasan <mailto:kys@microsoft.com>; Wei Liu <mailto:wei.liu@kernel.org>; Hawking Zhang <mailto:Hawking.Zhang@amd.com>
Subject: [EXTERNAL] Re: [RFC PATCH 0/4] DirectX on Linux
On Wed, May 20, 2020 at 12:42 AM Dave Airlie <mailto:airlied@gmail.com> wrote:
>
> On Wed, 20 May 2020 at 02:33, Sasha Levin <mailto:sashal@kernel.org> wrote:
> >
> > There is a blog post that goes into more detail about the bigger
> > picture, and walks through all the required pieces to make this
> > work. It is available here:
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fde
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvblogs.microsoft.com%2F&data=02%7C01%7Cspronovo%40microsoft.com%7C5e9c6fb0890844aef87c08d7fc91265e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637255572659005449&sdata=gVJvYw%2BRkrdugZw5LzWbYXzoAoT2KhVjRZLs%2Ft%2BG3aY%3D&reserved=0%2Fdirectx%2Fdirectx-heart-linux&data=02%7C01%7Cspronovo%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com%2F&data=02%7C01%7Cspronovo%40microsoft.com%7C5e9c6fb0890844aef87c08d7fc91265e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637255572659015442&sdata=0TfOBiUpDLctMq7EZmK7N3xdPFeqauzsXJPODRCErXU%3D&reserved=0%7C3f18e46192b24cccf6a008d7fc489063%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637255260910730243&sdata=IRRknzg%2F6Myzj3JXESN7GgmN6AcUV3DxhL95P%2ButtCw%3D&reserved=0 . The rest of this cover letter will focus on the Linux Kernel bits.
> >
> > Overview
> > ========
> >
> > This is the first draft of the Microsoft Virtual GPU (vGPU) driver.
> > The driver exposes a paravirtualized GPU to user mode applications
> > running in a virtual machine on a Windows host. This enables
> > hardware acceleration in environment such as WSL (Windows Subsystem
> > for Linux) where the Linux virtual machine is able to share the GPU
> > with the Windows host.
> >
> > The projection is accomplished by exposing the WDDM (Windows Display
> > Driver Model) interface as a set of IOCTL. This allows APIs and user
> > mode driver written against the WDDM GPU abstraction on Windows to
> > be ported to run within a Linux environment. This enables the port
> > of the
> > D3D12 and DirectML APIs as well as their associated user mode driver
> > to Linux. This also enables third party APIs, such as the popular
> > NVIDIA Cuda compute API, to be hardware accelerated within a WSL environment.
> >
> > Only the rendering/compute aspect of the GPU are projected to the
> > virtual machine, no display functionality is exposed. Further, at
> > this time there are no presentation integration. So although the
> > D3D12 API can be use to render graphics offscreen, there is no path
> > (yet) for pixel to flow from the Linux environment back onto the
> > Windows host desktop. This GPU stack is effectively side-by-side
> > with the native Linux graphics stack.
>
> Okay I've had some caffiene and absorbed some more of this.
>
> This is a driver that connects a binary blob interface in the Windows
> kernel drivers to a binary blob that you run inside a Linux guest.
> It's a binary transport between two binary pieces. Personally this
> holds little of interest to me, I can see why it might be nice to have
> this upstream, but I don't forsee any other Linux distributor ever
> enabling it or having to ship it, it's purely a WSL2 pipe. I'm not
> saying I'd be happy to see this in the tree, since I don't see the
> value of maintaining it upstream, but it probably should just exists
> in a drivers/hyperv type area.
Yup as-is (especially with the goal of this being aimed at ml/compute
only) drivers/hyperv sounds a bunch more reasonable than drivers/gpu.
> Having said that, I hit one stumbling block:
> "Further, at this time there are no presentation integration. "
>
> If we upstream this driver as-is into some hyperv specific place, and
> you decide to add presentation integration this is more than likely
> going to mean you will want to interact with dma-bufs and dma-fences.
> If the driver is hidden away in a hyperv place it's likely we won't
> even notice that feature landing until it's too late.
I've recently added regex matches to MAINTAINERS so we'll see dma_buf/fence/anything show up on dri-devel. So that part is solved hopefully.
> I would like to see a coherent plan for presentation support (not
> code, just an architectural diagram), because I think when you
> contemplate how that works it will change the picture of how this
> driver looks and intergrates into the rest of the Linux graphics
> ecosystem.
Yeah once we have the feature-creep to presentation support all the integration fun starts, with all the questions about "why does this not look like any other linux gpu driver". We have that already with nvidia insisting they just can't implement any of the upstream gpu uapi we have, but at least they're not in-tree, so not our problem from an upstream maintainership pov.
But once this dx12 pipe is landed and then we want to extend it it's still going to have all the "we can't ever release the sources to any of the parts we usually expect to be open for gpu drivers in upstream"
problems. Then we're stuck at a rather awkward point of why one vendor gets an exception and all the others dont.
> As-is I'd rather this didn't land under my purview, since I don't see
> the value this adds to the Linux ecosystem at all, and I think it's
> important when putting a burden on upstream that you provide some
> value.
Well there is some in the form of "more hw/platform support". But given that gpus evolved rather fast, including the entire integration ecosystem (it's by far not just the hw drivers that move quickly). So that value deprecates a lot faster than for other kernel subsystems.
And all that's left is the pain of not breaking anything without actually being able to evolve the overall stack in any meaningful way.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 -
+https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.f+fwll.ch%2F&data=02%7C01%7Cspronovo%40microsoft.com%7C3f18e46192b24c+ccf6a008d7fc489063%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6372552
+60910735230&sdata=hAIV1wJ29WF9IXTvJm3dr4StCwPzF0GdO2iWPyfnElg%3D&am
+p;reserved=0
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=02%7C01%7Cspronovo%40microsoft.com%7C5e9c6fb0890844aef87c08d7fc91265e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637255572659025436&sdata=me6tZuh9c4YKPTcOdY5%2FlgE%2B52usSI%2F5QRUCsCy%2FTfQ%3D&reserved=0
^ permalink raw reply
* RE: [PATCH v3 0/2] Fix PCI HyperV device error handling
From: Michael Kelley @ 2020-05-21 2:39 UTC (permalink / raw)
To: Lorenzo Pieralisi, Wei Hu
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui
In-Reply-To: <20200511112147.GD24954@e121166-lin.cambridge.arm.com>
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Monday, May 11, 2020 4:22 AM
>
> On Thu, May 07, 2020 at 01:01:26PM +0800, Wei Hu wrote:
> > This series better handles some PCI HyperV error cases in general
> > and for kdump case. Some of review comments from previous individual
> > patch reviews, including splitting into separate patches, have already
> > been incorporated. Thanks Lorenzo Pieralisi for the review and
> > suggestions, as well as Michael Kelley's contribution to the commit
> > log.
> >
> > Thanks,
> > Wei
> >
> >
> > Wei Hu (2):
> > PCI: hv: Fix the PCI HyperV probe failure path to release resource
> > properly
> > PCI: hv: Retry PCI bus D0 entry when the first attempt failed with
> > invalid device state
> >
> > drivers/pci/controller/pci-hyperv.c | 60 ++++++++++++++++++++++++++---
> > 1 file changed, 54 insertions(+), 6 deletions(-)
>
> Applied to pci/hv, thanks.
>
Lorenzo --
Will you be bringing these fixes into 5.7? The main fix is the 2nd patch, but
there wasn't a clear "Fixes:" tag to add because the problem is due more to
how Hyper-V operates than a bug in a previous Linux commit. We have a
customer experiencing the problem, so getting the fix into the main tree
sooner rather than later is helpful.
Thx,
Michael
^ permalink raw reply
* Re: [PATCH v3 0/2] Fix PCI HyperV device error handling
From: Lorenzo Pieralisi @ 2020-05-21 17:42 UTC (permalink / raw)
To: Michael Kelley
Cc: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui
In-Reply-To: <MW2PR2101MB1052A57E7BE9634821E29E4FD7B70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Thu, May 21, 2020 at 02:39:58AM +0000, Michael Kelley wrote:
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Sent: Monday, May 11, 2020 4:22 AM
> >
> > On Thu, May 07, 2020 at 01:01:26PM +0800, Wei Hu wrote:
> > > This series better handles some PCI HyperV error cases in general
> > > and for kdump case. Some of review comments from previous individual
> > > patch reviews, including splitting into separate patches, have already
> > > been incorporated. Thanks Lorenzo Pieralisi for the review and
> > > suggestions, as well as Michael Kelley's contribution to the commit
> > > log.
> > >
> > > Thanks,
> > > Wei
> > >
> > >
> > > Wei Hu (2):
> > > PCI: hv: Fix the PCI HyperV probe failure path to release resource
> > > properly
> > > PCI: hv: Retry PCI bus D0 entry when the first attempt failed with
> > > invalid device state
> > >
> > > drivers/pci/controller/pci-hyperv.c | 60 ++++++++++++++++++++++++++---
> > > 1 file changed, 54 insertions(+), 6 deletions(-)
> >
> > Applied to pci/hv, thanks.
> >
>
> Lorenzo --
>
> Will you be bringing these fixes into 5.7? The main fix is the 2nd patch, but
> there wasn't a clear "Fixes:" tag to add because the problem is due more to
> how Hyper-V operates than a bug in a previous Linux commit. We have a
> customer experiencing the problem, so getting the fix into the main tree
> sooner rather than later is helpful.
We usually send fixes at -rc* if the bug was introduced in the previous
release, at the moment this is v5.8 material not planned for such a late
-rc* (ie -rc7). We can send patches to stable and/or apply a Fixes: tag
if that can help when the commits hit mainline.
Lorenzo
^ permalink raw reply
* Re: [PATCH v6 09/10] arm64: efi: Export screen_info
From: Nikhil Mahale @ 2020-05-22 11:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Michael Kelley, Arnd Bergmann, Will Deacon, Catalin Marinas,
Mark Rutland, Marc Zyngier, Linux ARM, gregkh,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
Sunil Muthuswamy, Boqun Feng
In-Reply-To: <CAMj1kXEpryfqk5eKxB5NrDcriEBRQKEHnDVZNBMfB4DY=708fw@mail.gmail.com>
On 5/18/20 6:21 PM, Ard Biesheuvel wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, 18 May 2020 at 06:25, Nikhil Mahale <nmahale@nvidia.com> wrote:
>>
>> On 5/13/20 7:56 PM, Nikhil Mahale wrote:
>>> On 3/20/20 3:16 AM, Michael Kelley wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
>>>>>
>>>>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
>>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>>>>>>>>
>>>>>>>> The Hyper-V frame buffer driver may be built as a module, and
>>>>>>>> it needs access to screen_info. So export screen_info.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>>>>>>>
>>>>>>> Is there any chance of using a more modern KMS based driver for the screen
>>>>>>> than the old fbdev subsystem? I had hoped to one day completely remove
>>>>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
>>>>>>> architectures.
>>>>>>>
>>>>>>
>>>>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
>>>>>> frame buffer device. That driver builds and runs on both ARM64 and x86.
>>>>>>
>>>>>> I'm not knowledgeable about video/graphics drivers, but when you
>>>>>> say "a more modern KMS based driver", are you meaning one based on
>>>>>> DRM & KMS? Does DRM make sense for a "dumb" frame buffer device?
>>>>>> Are there any drivers that would be a good pattern to look at?
>>>>>
>>>>> It used to be a lot harder to write a DRM driver compared to an fbdev
>>>>> driver, but this has changed to the opposite over the years.
>>>>>
>>>>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
>>>>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
>>>>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
>>>>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
>>>>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
>>>>>
>>>>
>>>> Thanks for the pointers, especially for the other hypervisors.
>>>>
>>> Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
>>> I had configured by email client incorrectly to reply.
>>>
>>> screen_info is still useful with a modern KMS-based driver. It exposes
>>> the mode parameters that the GOP driver chose. This information is
>>> needed to implement seamless or glitchless boot, by both ensuring that
>>> the scanout parameters don't change and being able to read back the
>>> scanout image to populate the initial contents of the new surface.
>>>
>>> This works today on arches which implement (U)EFI and export
>>> screen_info, including x86 and powerpc, but doesn't work on arm or
>>> arm64. As arm64 systems that implement UEFI with real GOP drivers
>>> become more prevalent, it would be nice to be have these features there
>>> as well.
>>
>> In addition to this, even if a driver doesn't implement a framebuffer
>> console, or if it does but has an option to disable it, the driver still
>> needs to know whether the EFI console is using resources on the GPU so
>> it can avoid clobbering them. For example screen_info provides information
>> like offset and size of EFI console, using this information driver can
>> reserve memory used by console and prevent corruption on it.
>>
>> I think arm64 should export screen_info.
>>
>
> If there are reasons why KMS or fbdev drivers may need to access the
> information in screen_info, it should be exported. I don't think that
> is under debate here.
>
Hi Ard, thanks for your feedback. If my understanding is correct,
you are agree to export screen_info. Can you provide guidance on how can
we proceed here? are you willing to accept this current patch as-is or
would you like me to re-submit the patch with the additional rationale
provided?
Thanks,
Nikhil Mahale
^ permalink raw reply
* Re: [PATCH v6 09/10] arm64: efi: Export screen_info
From: Ard Biesheuvel @ 2020-05-22 11:32 UTC (permalink / raw)
To: Nikhil Mahale
Cc: Michael Kelley, Arnd Bergmann, Will Deacon, Catalin Marinas,
Mark Rutland, Marc Zyngier, Linux ARM, gregkh,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
Sunil Muthuswamy, Boqun Feng
In-Reply-To: <e8850c9b-965a-9ed4-fb22-f41de5f72b60@nvidia.com>
On Fri, 22 May 2020 at 13:15, Nikhil Mahale <nmahale@nvidia.com> wrote:
>
> On 5/18/20 6:21 PM, Ard Biesheuvel wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, 18 May 2020 at 06:25, Nikhil Mahale <nmahale@nvidia.com> wrote:
> >>
> >> On 5/13/20 7:56 PM, Nikhil Mahale wrote:
> >>> On 3/20/20 3:16 AM, Michael Kelley wrote:
> >>>> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
> >>>>>
> >>>>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> >>>>>> From: Arnd Bergmann <arnd@arndb.de>
> >>>>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >>>>>>>>
> >>>>>>>> The Hyper-V frame buffer driver may be built as a module, and
> >>>>>>>> it needs access to screen_info. So export screen_info.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >>>>>>>
> >>>>>>> Is there any chance of using a more modern KMS based driver for the screen
> >>>>>>> than the old fbdev subsystem? I had hoped to one day completely remove
> >>>>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> >>>>>>> architectures.
> >>>>>>>
> >>>>>>
> >>>>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> >>>>>> frame buffer device. That driver builds and runs on both ARM64 and x86.
> >>>>>>
> >>>>>> I'm not knowledgeable about video/graphics drivers, but when you
> >>>>>> say "a more modern KMS based driver", are you meaning one based on
> >>>>>> DRM & KMS? Does DRM make sense for a "dumb" frame buffer device?
> >>>>>> Are there any drivers that would be a good pattern to look at?
> >>>>>
> >>>>> It used to be a lot harder to write a DRM driver compared to an fbdev
> >>>>> driver, but this has changed to the opposite over the years.
> >>>>>
> >>>>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
> >>>>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
> >>>>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
> >>>>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
> >>>>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
> >>>>>
> >>>>
> >>>> Thanks for the pointers, especially for the other hypervisors.
> >>>>
> >>> Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
> >>> I had configured by email client incorrectly to reply.
> >>>
> >>> screen_info is still useful with a modern KMS-based driver. It exposes
> >>> the mode parameters that the GOP driver chose. This information is
> >>> needed to implement seamless or glitchless boot, by both ensuring that
> >>> the scanout parameters don't change and being able to read back the
> >>> scanout image to populate the initial contents of the new surface.
> >>>
> >>> This works today on arches which implement (U)EFI and export
> >>> screen_info, including x86 and powerpc, but doesn't work on arm or
> >>> arm64. As arm64 systems that implement UEFI with real GOP drivers
> >>> become more prevalent, it would be nice to be have these features there
> >>> as well.
> >>
> >> In addition to this, even if a driver doesn't implement a framebuffer
> >> console, or if it does but has an option to disable it, the driver still
> >> needs to know whether the EFI console is using resources on the GPU so
> >> it can avoid clobbering them. For example screen_info provides information
> >> like offset and size of EFI console, using this information driver can
> >> reserve memory used by console and prevent corruption on it.
> >>
> >> I think arm64 should export screen_info.
> >>
> >
> > If there are reasons why KMS or fbdev drivers may need to access the
> > information in screen_info, it should be exported. I don't think that
> > is under debate here.
> >
>
> Hi Ard, thanks for your feedback. If my understanding is correct,
> you are agree to export screen_info. Can you provide guidance on how can
> we proceed here? are you willing to accept this current patch as-is or
> would you like me to re-submit the patch with the additional rationale
> provided?
>
Please (re-)submit it along with the code that actually makes use of it.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page reporting
From: Sunil Muthuswamy @ 2020-05-22 16:40 UTC (permalink / raw)
To: Wei Liu, Alexander Duyck
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20200520090158.4x4lkbssm7ncirn7@liuwe-devbox-debian-v2.j3c5onc20sse1dnehy4noqpfcg.zx.internal.cloudapp.net>
> > + if (hv_do_hypercall(HV_EXT_CALL_QUERY_CAPABILITIES, NULL, cap) ==
> > + HV_STATUS_SUCCESS)
>
> You're using the input page as the output parameter. Ideally we should
> introduce hyperv_pcpu_output_arg page, but that would waste one page per
> cpu just for this one call.
>
> So for now I think this setup is fine, but I would like to add the
> following comment.
>
> /*
> * Repurpose the input_arg page to accept output from Hyper-V for
> * now because this is the only call that needs output from the
> * hypervisor. It should be fixed properly by introducing an
> * output_arg page once we have more places that require output.
> */
Sounds good. Will add it in v2.
> > +#ifdef CONFIG_PAGE_REPORTING
> > +static int hv_free_page_report(struct page_reporting_dev_info *pr_dev_info,
> > + struct scatterlist *sgl, unsigned int nents)
> > +{
> > + unsigned long flags;
> > + struct hv_memory_hint *hint;
> > + int i;
> > + u64 status;
> > + struct scatterlist *sg;
> > +
> > + WARN_ON(nents > HV_MAX_GPA_PAGE_RANGES);
>
> Should we return -ENOSPC here?
This is more of an assert because PAGE_REPORTING_CAPACITY is set to 32 and has
already been checked that it is < HV_MAX_GPA_PAGE_RANGES in enable_page_reporting.
> > + hint->type = HV_EXT_MEMORY_HEAT_HINT_TYPE_COLD_DISCARD;
> > + hint->reserved = 0;
> > + for (i = 0, sg = sgl; sg; sg = sg_next(sg), i++) {
> > + int order;
> > + union hv_gpa_page_range *range;
> > +
>
> Unfortunately I can't find the semantics of this hypercall in TLFS 6, so
> I have a few questions here.
This structure is not specific to this hypercall.
>
> > + order = get_order(sg->length);
> > + range = &hint->ranges[i];
> > + range->address_space = 0;
>
> I guess this means all address spaces?
'address_space' is being used here just as a zero initializer for the union. I think the use
of 'address_space' in the definition of hv_gpa_page_range is not appropriate. This struct is
defined in the TLFS 6.0 with the same name, if you want to look it up.
>
> > + range->page.largepage = 1;
>
> What effect does this have? What if the page is a 4k page?
Page reporting infrastructure doesn't report 4k pages today, but only huge pages (see
PAGE_REPORTING_MIN_ORDER in page_reporting.h). Additionally, the Hyper-V hypervisor
only supports reporting of 2M pages and above. The current code assumes that the minimum
order will be 9 i.e 2M pages and above.
If we feel that this could change in the future, or an implementation detail that should be
protected against, I can add some checks in hv_balloon.c. But, in that case, the ballon driver
should be able to query the page reporting min order somehow, which it is currently, since it is
private.
Alexander, do you have any suggestions or feedback here?
>
> > + range->page.additional_pages = (1ull << (order - 9)) - 1;
>
> What is 9 here? Is there a macro name *ORDER that you can use?
It is to determine the count of 2M pages. I will define a macro.
>
> Wei.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page reporting
From: Sunil Muthuswamy @ 2020-05-22 17:02 UTC (permalink / raw)
To: vkuznets
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Tianyu Lan
In-Reply-To: <87ftbvt21h.fsf@vitty.brq.redhat.com>
> As the only usage of this function looks like
> if (!(hv_query_ext_cap() & HV_EXT_CAPABILITY_MEMORY_COLD_DISCARD_HINT))
>
> I would've change the interface to
>
> bool hv_query_ext_cap(u64 cap)
>
> so the usage would look like
>
> if (!(hv_query_ext_cap(HV_EXT_CAPABILITY_MEMORY_COLD_DISCARD_HINT))
Good idea. Will do in v2.
> > ms_hyperv.features = cpuid_eax(HYPERV_CPUID_FEATURES);
> > + ms_hyperv.b_features = cpuid_ebx(HYPERV_CPUID_FEATURES);
> > ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
> > ms_hyperv.hints = cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
> >
> > - pr_info("Hyper-V: features 0x%x, hints 0x%x, misc 0x%x\n",
> > - ms_hyperv.features, ms_hyperv.hints, ms_hyperv.misc_features);
> > + pr_info("Hyper-V: features 0x%x, additional features: 0x%x, hints 0x%x, misc 0x%x\n",
> > + ms_hyperv.features, ms_hyperv.b_features, ms_hyperv.hints,
> > + ms_hyperv.misc_features);
>
> HYPERV_CPUID_FEATURES(0x40000003) EAX and EBX correspond to Partition
> Privilege Flags (TLFS), I'd suggest to take the opportunity and rename
> this to something like 'privilege flags low=0x%x high=0x%x'.
>
> Also, I don't quite like 'ms_hyperv.b_features' as I'll always have to
> look at what it's being assigned to understand what it holds. I'd even
> suggest to rename ms_hyperv.features to ms_hyperv.priv_low and
> ms_hyperv.b_features tp ms_hyperv.priv_high. Or maybe even better, pack
> them to the same 'u64 ms_hyperv.privileges'.
Good idea. I will make the change to rename this to 'priv_high' in v2. I like the idea of
combining 'features' & 'priv_high' to a u64, but that would be a cleanup change and a
separate patch.
>
> Why is largepage always '1'?
I have responded to a similar question by Wei. Page reporting only supports huge pages
and, so does the Hyper-V hypervisor. Let's follow this there.
>
> > + range->page.additional_pages = (1ull << (order - 9)) - 1;
> > + range->base_large_pfn = page_to_pfn(sg_page(sg)) >> 9;
>
> What is '9'? Could you please define it through PAGE_*/HPAGE_* macro?
Yes, I will define a macro. Essentially, it is to get a count of 2M pages.
> Nit: you could've just used
>
> if (status & HV_HYPERCALL_RESULT_MASK != HV_STATUS_SUCCESS) {
Sure, coming in v2.
> ...
>
> > + pr_err("Cold memory discard hypercall failed with status %llx\n",
> > + status);
> > + return -1;
>
> -EFAULT or something like it maybe?
Coming in v2.
> > +#ifdef CONFIG_PAGE_REPORTING
> > + if (enable_page_reporting() < 0)
> > + goto probe_error;
>
> Why? The hyperv-balloon driver itself may still be functional and you
> already set dm_device.pr_dev_info.report to NULL.
An error here would reflect an internal error and should not happen and
it was to make it easy to catch such errors, which are otherwise difficult
with just a print. But, the code should follow the general spirit. I will change
this in v2.
>
> > enum HV_GENERIC_SET_FORMAT {
> > HV_GENERIC_SET_SPARSE_4K,
> > HV_GENERIC_SET_ALL,
> > @@ -371,6 +379,12 @@ union hv_gpa_page_range {
>
> There is a comment before this structure:
>
> /* HvFlushGuestPhysicalAddressList hypercall */
>
> which is now obsolete.
I will add that it also applies to 'HvExtCallMemoryHeatHint' hypercall also.
>
> > u64 largepage:1;
> > u64 basepfn:52;
> > } page;
> > + struct {
> > + u64:12;
>
> What is this unnamed member? Another 'reserved', 'pad'? Let's name it.
Sure, coming in v2.
>
> > + u64 page_size:1;
> > + u64 reserved:8;
> > + u64 base_large_pfn:43;
> > + };
>
> Please name this structure in the union.
Sure, coming in v2.
> > + */
> > +#define HV_MAX_GPA_PAGE_RANGES ((PAGE_SIZE - sizeof(u64)) / \
> > + sizeof(union hv_gpa_page_range))
> > +
>
> The name HV_MAX_GPA_PAGE_RANGES sounds too generic and I think this is
> specific to the HvExtCallMemoryHeatHint hypercall as other hypercalls
> may have a different header length.
>
Good idea to rename. Coming in v2.
> > +/* HvExtCallMemoryHeatHint hypercall */
> > +#define HV_EXT_MEMORY_HEAT_HINT_TYPE_COLD_DISCARD 2
> > +struct hv_memory_hint {
> > + u64 type:2;
> > + u64 reserved:62;
> > + union hv_gpa_page_range ranges[1];
>
> Why '[1]' and not '[]'? If it was '[]' you could've used 'sizeof(struct
> hv_memory_hint)' in HV_MAX_GPA_PAGE_RANGES macro definition instead of
> 'sizeof(u64)'.
>
Good idea, coming in v2.
- Sunil
^ permalink raw reply
* [PATCH] VMBus channel interrupts reassignment - Fixes
From: Andrea Parri (Microsoft) @ 2020-05-22 17:18 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Andrea Parri (Microsoft)
Two fixes on top of the channel interrupts reassignment series, both
addressing race conditions between the initialization of the target
CPUs and the CPU hot removal path. (Fixes: tags refer to commit IDs
from the hyperv tree and should be considered as placeholders; please
let me know how you'd prefer to handle these...)
Thanks,
Andrea
Andrea Parri (Microsoft) (2):
Drivers: hv: vmbus: Resolve race between init_vp_index() and CPU
hotplug
Drivers: hv: vmbus: Resolve more races involving init_vp_index()
drivers/hv/channel_mgmt.c | 66 +++++++++++++++++++--------------------
drivers/hv/hyperv_vmbus.h | 48 ++++++++++++++++++++++++++++
drivers/hv/vmbus_drv.c | 19 +++++++----
include/linux/hyperv.h | 7 +++++
4 files changed, 101 insertions(+), 39 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 1/2] Drivers: hv: vmbus: Resolve race between init_vp_index() and CPU hotplug
From: Andrea Parri (Microsoft) @ 2020-05-22 17:19 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Andrea Parri (Microsoft)
In-Reply-To: <20200522171901.204127-1-parri.andrea@gmail.com>
vmbus_process_offer() does two things (among others):
1) first, it sets the channel's target CPU with cpu_hotplug_lock;
2) it then adds the channel to the channel list(s) with channel_mutex.
Since cpu_hotplug_lock is released before (2), the channel's target CPU
(as designated in (1)) can be deemed "free" by hv_synic_cleanup() and go
offline before the channel is added to the list.
Fix the race condition by "extending" the cpu_hotplug_lock critical
section to include (2) (and (1)), nesting the channel_mutex critical
section within the cpu_hotplug_lock critical section as done elsewhere
(hv_synic_cleanup(), target_cpu_store()) in the hyperv drivers code.
Move even further by extending the channel_mutex critical section to
include (1) (and (2)): this change allows to remove (the now redundant)
bind_channel_to_cpu_lock, and generally simplifies the handling of the
target CPUs (that are now always modified with channel_mutex held).
Fixes: d570aec0f2154e ("Drivers: hv: vmbus: Synchronize init_vp_index() vs. CPU hotplug")
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 46 +++++++++++++++------------------------
1 file changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index fde806d6525b0..89eaacf069a80 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -554,26 +554,34 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
bool fnew = true;
/*
- * Initialize the target_CPU before inserting the channel in
- * the chn_list and sc_list lists, within the channel_mutex
- * critical section:
+ * Synchronize vmbus_process_offer() and CPU hotplugging:
*
* CPU1 CPU2
*
- * [vmbus_process_offer()] [hv_syninc_cleanup()]
+ * [vmbus_process_offer()] [Hot removal of the CPU]
*
- * STORE target_cpu LOCK channel_mutex
- * LOCK channel_mutex SEARCH chn_list
- * INSERT chn_list LOAD target_cpu
- * UNLOCK channel_mutex UNLOCK channel_mutex
+ * CPU_READ_LOCK CPUS_WRITE_LOCK
+ * LOAD cpu_online_mask SEARCH chn_list
+ * STORE target_cpu LOAD target_cpu
+ * INSERT chn_list STORE cpu_online_mask
+ * CPUS_READ_UNLOCK CPUS_WRITE_UNLOCK
+ *
+ * Forbids: CPU1's LOAD from *not* seing CPU2's STORE &&
+ * CPU2's SEARCH from *not* seeing CPU1's INSERT
*
* Forbids: CPU2's SEARCH from seeing CPU1's INSERT &&
* CPU2's LOAD from *not* seing CPU1's STORE
*/
- init_vp_index(newchannel, hv_get_dev_type(newchannel));
+ cpus_read_lock();
+ /*
+ * Serializes the modifications of the chn_list list as well as
+ * the accesses to next_numa_node_id in init_vp_index().
+ */
mutex_lock(&vmbus_connection.channel_mutex);
+ init_vp_index(newchannel, hv_get_dev_type(newchannel));
+
/* Remember the channels that should be cleaned up upon suspend. */
if (is_hvsock_channel(newchannel) || is_sub_channel(newchannel))
atomic_inc(&vmbus_connection.nr_chan_close_on_suspend);
@@ -623,6 +631,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
vmbus_channel_map_relid(newchannel);
mutex_unlock(&vmbus_connection.channel_mutex);
+ cpus_read_unlock();
/*
* vmbus_process_offer() mustn't call channel->sc_creation_callback()
@@ -655,13 +664,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
* We use this state to statically distribute the channel interrupt load.
*/
static int next_numa_node_id;
-/*
- * init_vp_index() accesses global variables like next_numa_node_id, and
- * it can run concurrently for primary channels and sub-channels: see
- * vmbus_process_offer(), so we need the lock to protect the global
- * variables.
- */
-static DEFINE_SPINLOCK(bind_channel_to_cpu_lock);
/*
* Starting with Win8, we can statically distribute the incoming
@@ -700,15 +702,6 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
return;
}
- /* No CPUs can come up or down during this. */
- cpus_read_lock();
-
- /*
- * Serializes the accesses to the global variable next_numa_node_id.
- * See also the header comment of the spin lock declaration.
- */
- spin_lock(&bind_channel_to_cpu_lock);
-
while (true) {
numa_node = next_numa_node_id++;
if (numa_node == nr_node_ids) {
@@ -739,9 +732,6 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
channel->target_cpu = target_cpu;
channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
- spin_unlock(&bind_channel_to_cpu_lock);
- cpus_read_unlock();
-
free_cpumask_var(available_mask);
}
--
2.25.1
^ permalink raw reply related
* [PATCH 2/2] Drivers: hv: vmbus: Resolve more races involving init_vp_index()
From: Andrea Parri (Microsoft) @ 2020-05-22 17:19 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Andrea Parri (Microsoft)
In-Reply-To: <20200522171901.204127-1-parri.andrea@gmail.com>
init_vp_index() uses the (per-node) hv_numa_map[] masks to record the
CPUs allocated for channel interrupts at a given time, and distribute
the performance-critical channels across the available CPUs: in part.,
the mask of "candidate" target CPUs in a given NUMA node, for a newly
offered channel, is determined by XOR-ing the node's CPU mask and the
node's hv_numa_map. This operation/mechanism assumes that no offline
CPUs is set in the hv_numa_map mask, an assumption that does not hold
since such mask is currently not updated when a channel is removed or
assigned to a different CPU.
To address the issues described above, this adds hooks in the channel
removal path (hv_process_channel_removal()) and in target_cpu_store()
in order to clear, resp. to update, the hv_numa_map[] masks as needed.
This also adds a (missed) update of the masks in init_vp_index() (cf.,
e.g., the memory-allocation failure path in this function).
Like in the case of init_vp_index(), such hooks require to determine
if the given channel is performance critical. init_vp_index() does
this by parsing the channel's offer, it can not rely on the device
data structure (device_obj) to retrieve such information because the
device data structure has not been allocated/linked with the channel
by the time that init_vp_index() executes. A similar situation may
hold in hv_is_alloced_cpu() (defined below); the adopted approach is
to "cache" the device type of the channel, as computed by parsing the
channel's offer, in the channel structure itself.
Fixes: 7527810573436f ("Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL message type")
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 22 +++++++++++++-----
drivers/hv/hyperv_vmbus.h | 48 +++++++++++++++++++++++++++++++++++++++
drivers/hv/vmbus_drv.c | 19 +++++++++++-----
include/linux/hyperv.h | 7 ++++++
4 files changed, 84 insertions(+), 12 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 89eaacf069a80..417a95e5094dd 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -24,9 +24,9 @@
#include "hyperv_vmbus.h"
-static void init_vp_index(struct vmbus_channel *channel, u16 dev_type);
+static void init_vp_index(struct vmbus_channel *channel);
-static const struct vmbus_device vmbus_devs[] = {
+const struct vmbus_device vmbus_devs[] = {
/* IDE */
{ .dev_type = HV_IDE,
HV_IDE_GUID,
@@ -431,6 +431,13 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
+ /*
+ * If this is a "perf" channel, updates the hv_numa_map[] masks so that
+ * init_vp_index() can (re-)use the CPU.
+ */
+ if (hv_is_perf_channel(channel))
+ hv_clear_alloced_cpu(channel->target_cpu);
+
/*
* Upon suspend, an in-use hv_sock channel is marked as "rescinded" and
* the relid is invalidated; after hibernation, when the user-space app
@@ -497,7 +504,7 @@ static void vmbus_add_channel_work(struct work_struct *work)
if (!newchannel->device_obj)
goto err_deq_chan;
- newchannel->device_obj->device_id = hv_get_dev_type(newchannel);
+ newchannel->device_obj->device_id = newchannel->device_id;
/*
* Add the new device to the bus. This will kick off device-driver
* binding which eventually invokes the device driver's AddDevice()
@@ -580,7 +587,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
*/
mutex_lock(&vmbus_connection.channel_mutex);
- init_vp_index(newchannel, hv_get_dev_type(newchannel));
+ init_vp_index(newchannel);
/* Remember the channels that should be cleaned up upon suspend. */
if (is_hvsock_channel(newchannel) || is_sub_channel(newchannel))
@@ -676,9 +683,9 @@ static int next_numa_node_id;
* evenly among all the available NUMA nodes. Once the node is assigned,
* we will assign the CPU based on a simple round robin scheme.
*/
-static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
+static void init_vp_index(struct vmbus_channel *channel)
{
- bool perf_chn = vmbus_devs[dev_type].perf_device;
+ bool perf_chn = hv_is_perf_channel(channel);
cpumask_var_t available_mask;
struct cpumask *alloced_mask;
u32 target_cpu;
@@ -699,6 +706,8 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
channel->target_cpu = VMBUS_CONNECT_CPU;
channel->target_vp =
hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
+ if (perf_chn)
+ hv_set_alloced_cpu(VMBUS_CONNECT_CPU);
return;
}
@@ -862,6 +871,7 @@ static void vmbus_setup_channel_state(struct vmbus_channel *channel,
sizeof(struct vmbus_channel_offer_channel));
channel->monitor_grp = (u8)offer->monitorid / 32;
channel->monitor_bit = (u8)offer->monitorid % 32;
+ channel->device_id = hv_get_dev_type(channel);
}
/*
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 5e5cebe5d048f..40e2b9f91163c 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -395,6 +395,54 @@ enum delay {
MESSAGE_DELAY = 1,
};
+extern const struct vmbus_device vmbus_devs[];
+
+static inline bool hv_is_perf_channel(struct vmbus_channel *channel)
+{
+ return vmbus_devs[channel->device_id].perf_device;
+}
+
+static inline bool hv_is_alloced_cpu(unsigned int cpu)
+{
+ struct vmbus_channel *channel, *sc;
+
+ lockdep_assert_held(&vmbus_connection.channel_mutex);
+ /*
+ * List additions/deletions as well as updates of the target CPUs are
+ * protected by channel_mutex.
+ */
+ list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
+ if (!hv_is_perf_channel(channel))
+ continue;
+ if (channel->target_cpu == cpu)
+ return true;
+ list_for_each_entry(sc, &channel->sc_list, sc_list) {
+ if (sc->target_cpu == cpu)
+ return true;
+ }
+ }
+ return false;
+}
+
+static inline void hv_set_alloced_cpu(unsigned int cpu)
+{
+ cpumask_set_cpu(cpu, &hv_context.hv_numa_map[cpu_to_node(cpu)]);
+}
+
+static inline void hv_clear_alloced_cpu(unsigned int cpu)
+{
+ if (hv_is_alloced_cpu(cpu))
+ return;
+ cpumask_clear_cpu(cpu, &hv_context.hv_numa_map[cpu_to_node(cpu)]);
+}
+
+static inline void hv_update_alloced_cpus(unsigned int old_cpu,
+ unsigned int new_cpu)
+{
+ hv_set_alloced_cpu(new_cpu);
+ hv_clear_alloced_cpu(old_cpu);
+}
+
#ifdef CONFIG_HYPERV_TESTING
int hv_debug_add_dev_dir(struct hv_device *dev);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index c2a4a7c0b99a0..47747755d2e1d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1687,8 +1687,8 @@ static ssize_t target_cpu_show(struct vmbus_channel *channel, char *buf)
static ssize_t target_cpu_store(struct vmbus_channel *channel,
const char *buf, size_t count)
{
+ u32 target_cpu, origin_cpu;
ssize_t ret = count;
- u32 target_cpu;
if (vmbus_proto_version < VERSION_WIN10_V4_1)
return -EIO;
@@ -1741,7 +1741,8 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
goto cpu_store_unlock;
}
- if (channel->target_cpu == target_cpu)
+ origin_cpu = channel->target_cpu;
+ if (target_cpu == origin_cpu)
goto cpu_store_unlock;
if (vmbus_send_modifychannel(channel->offermsg.child_relid,
@@ -1763,14 +1764,20 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
* in on a CPU that is different from the channel target_cpu value.
*/
- if (channel->change_target_cpu_callback)
- (*channel->change_target_cpu_callback)(channel,
- channel->target_cpu, target_cpu);
-
channel->target_cpu = target_cpu;
channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
channel->numa_node = cpu_to_node(target_cpu);
+ /* See init_vp_index(). */
+ if (hv_is_perf_channel(channel))
+ hv_update_alloced_cpus(origin_cpu, target_cpu);
+
+ /* Currently set only for storvsc channels. */
+ if (channel->change_target_cpu_callback) {
+ (*channel->change_target_cpu_callback)(channel,
+ origin_cpu, target_cpu);
+ }
+
cpu_store_unlock:
mutex_unlock(&vmbus_connection.channel_mutex);
cpus_read_unlock();
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index d783847d8cb46..40df3103e890b 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -901,6 +901,13 @@ struct vmbus_channel {
bool probe_done;
+ /*
+ * Cache the device ID here for easy access; this is useful, in
+ * particular, in situations where the channel's device_obj has
+ * not been allocated/initialized yet.
+ */
+ u16 device_id;
+
/*
* We must offload the handling of the primary/sub channels
* from the single-threaded vmbus_connection.work_queue to
--
2.25.1
^ permalink raw reply related
* RE: [PATCH 1/2] Drivers: hv: vmbus: Resolve race between init_vp_index() and CPU hotplug
From: Michael Kelley @ 2020-05-22 18:34 UTC (permalink / raw)
To: Andrea Parri (Microsoft), linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
In-Reply-To: <20200522171901.204127-2-parri.andrea@gmail.com>
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Friday, May 22, 2020 10:19 AM
>
> vmbus_process_offer() does two things (among others):
>
> 1) first, it sets the channel's target CPU with cpu_hotplug_lock;
> 2) it then adds the channel to the channel list(s) with channel_mutex.
>
> Since cpu_hotplug_lock is released before (2), the channel's target CPU
> (as designated in (1)) can be deemed "free" by hv_synic_cleanup() and go
> offline before the channel is added to the list.
>
> Fix the race condition by "extending" the cpu_hotplug_lock critical
> section to include (2) (and (1)), nesting the channel_mutex critical
> section within the cpu_hotplug_lock critical section as done elsewhere
> (hv_synic_cleanup(), target_cpu_store()) in the hyperv drivers code.
>
> Move even further by extending the channel_mutex critical section to
> include (1) (and (2)): this change allows to remove (the now redundant)
> bind_channel_to_cpu_lock, and generally simplifies the handling of the
> target CPUs (that are now always modified with channel_mutex held).
>
> Fixes: d570aec0f2154e ("Drivers: hv: vmbus: Synchronize init_vp_index() vs. CPU hotplug")
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel_mgmt.c | 46 +++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 28 deletions(-)
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH 2/2] Drivers: hv: vmbus: Resolve more races involving init_vp_index()
From: Michael Kelley @ 2020-05-22 18:41 UTC (permalink / raw)
To: Andrea Parri (Microsoft), linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
In-Reply-To: <20200522171901.204127-3-parri.andrea@gmail.com>
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
>
> init_vp_index() uses the (per-node) hv_numa_map[] masks to record the
> CPUs allocated for channel interrupts at a given time, and distribute
> the performance-critical channels across the available CPUs: in part.,
> the mask of "candidate" target CPUs in a given NUMA node, for a newly
> offered channel, is determined by XOR-ing the node's CPU mask and the
> node's hv_numa_map. This operation/mechanism assumes that no offline
> CPUs is set in the hv_numa_map mask, an assumption that does not hold
> since such mask is currently not updated when a channel is removed or
> assigned to a different CPU.
>
> To address the issues described above, this adds hooks in the channel
> removal path (hv_process_channel_removal()) and in target_cpu_store()
> in order to clear, resp. to update, the hv_numa_map[] masks as needed.
> This also adds a (missed) update of the masks in init_vp_index() (cf.,
> e.g., the memory-allocation failure path in this function).
>
> Like in the case of init_vp_index(), such hooks require to determine
> if the given channel is performance critical. init_vp_index() does
> this by parsing the channel's offer, it can not rely on the device
> data structure (device_obj) to retrieve such information because the
> device data structure has not been allocated/linked with the channel
> by the time that init_vp_index() executes. A similar situation may
> hold in hv_is_alloced_cpu() (defined below); the adopted approach is
> to "cache" the device type of the channel, as computed by parsing the
> channel's offer, in the channel structure itself.
>
> Fixes: 7527810573436f ("Drivers: hv: vmbus: Introduce the
> CHANNELMSG_MODIFYCHANNEL message type")
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel_mgmt.c | 22 +++++++++++++-----
> drivers/hv/hyperv_vmbus.h | 48 +++++++++++++++++++++++++++++++++++++++
> drivers/hv/vmbus_drv.c | 19 +++++++++++-----
> include/linux/hyperv.h | 7 ++++++
> 4 files changed, 84 insertions(+), 12 deletions(-)
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page reporting
From: Alexander Duyck @ 2020-05-22 20:11 UTC (permalink / raw)
To: Sunil Muthuswamy
Cc: Wei Liu, Alexander Duyck, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, Michael Kelley, Tianyu Lan,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <SN4PR2101MB08804F99992085C64982C821C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com>
On Fri, May 22, 2020 at 9:42 AM Sunil Muthuswamy <sunilmut@microsoft.com> wrote:
>
[...]
> >
> > > + order = get_order(sg->length);
> > > + range = &hint->ranges[i];
> > > + range->address_space = 0;
> >
> > I guess this means all address spaces?
>
> 'address_space' is being used here just as a zero initializer for the union. I think the use
> of 'address_space' in the definition of hv_gpa_page_range is not appropriate. This struct is
> defined in the TLFS 6.0 with the same name, if you want to look it up.
>
> >
> > > + range->page.largepage = 1;
> >
> > What effect does this have? What if the page is a 4k page?
>
> Page reporting infrastructure doesn't report 4k pages today, but only huge pages (see
> PAGE_REPORTING_MIN_ORDER in page_reporting.h). Additionally, the Hyper-V hypervisor
> only supports reporting of 2M pages and above. The current code assumes that the minimum
> order will be 9 i.e 2M pages and above.
> If we feel that this could change in the future, or an implementation detail that should be
> protected against, I can add some checks in hv_balloon.c. But, in that case, the ballon driver
> should be able to query the page reporting min order somehow, which it is currently, since it is
> private.
> Alexander, do you have any suggestions or feedback here?
For now we are keeping the limit to order 9 for a couple reasons. The
first being that we don't want to trigger the breaking apart of
transparent huge pages on the host, and the second being for
performance as it is better to report larger pages rather than smaller
ones.
If we were to enable bringing the value down lower we would likely
make it a part of the page reporting dev info structure and would have
to be set at initialization time. That way the device itself could
configure the minimum value. I don't see the value itself being
lowered without adding an option like that since it would likely cause
issues for several different reasons going forward though. If nothing
else you could do a BUILD_BUG_ON that would assert if
PAGE_REPORTING_MIN_ORDER was anything other than the same size as the
"large_page" size referenced above.
> >
> > > + range->page.additional_pages = (1ull << (order - 9)) - 1;
> >
> > What is 9 here? Is there a macro name *ORDER that you can use?
>
> It is to determine the count of 2M pages. I will define a macro.
Is this the only spot where you are using order? Instead of converting
the length to an order why not just divide it by the 2M page size? I
would think that would be faster than having to do something like
having to call get_order on the length? Also instead of using "9" it
might make more sense if you have a define somewhere that says what
"large_page" size actually is. Then you could just divide by that
which should be translated into a shift which is fast and cheap.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH] x86/Hyper-V: Support for free page reporting
From: Michael Kelley @ 2020-05-23 0:40 UTC (permalink / raw)
To: Sunil Muthuswamy, Wei Liu, Alexander Duyck
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <SN4PR2101MB08804F99992085C64982C821C0B40@SN4PR2101MB0880.namprd21.prod.outlook.com>
From: Sunil Muthuswamy <sunilmut@microsoft.com> Sent: Friday, May 22, 2020 9:40 AM
>
> > > + if (hv_do_hypercall(HV_EXT_CALL_QUERY_CAPABILITIES, NULL, cap) ==
> > > + HV_STATUS_SUCCESS)
> >
> > You're using the input page as the output parameter. Ideally we should
> > introduce hyperv_pcpu_output_arg page, but that would waste one page per
> > cpu just for this one call.
> >
> > So for now I think this setup is fine, but I would like to add the
> > following comment.
> >
> > /*
> > * Repurpose the input_arg page to accept output from Hyper-V for
> > * now because this is the only call that needs output from the
> > * hypervisor. It should be fixed properly by introducing an
> > * output_arg page once we have more places that require output.
> > */
>
> Sounds good. Will add it in v2.
>
Note that the only real requirement for the output parameter to hypercalls
is that it not cross a page boundary. Since '*cap' is only 64-bits, you can
declare it as a static variable or even as a local on the stack. It will
naturally be aligned (or can add __aligned(8) to be explicit??), so it won't
cross a page boundary. Then you can skip using the per-cpu input arg
altogether, along with the associated local_irq_save()/restore().
Michael
^ permalink raw reply
* Re: [PATCH] VMBus channel interrupts reassignment - Fixes
From: Wei Liu @ 2020-05-23 9:04 UTC (permalink / raw)
To: Andrea Parri (Microsoft)
Cc: linux-hyperv, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, Michael Kelley
In-Reply-To: <20200522171901.204127-1-parri.andrea@gmail.com>
On Fri, May 22, 2020 at 07:18:59PM +0200, Andrea Parri (Microsoft) wrote:
> Two fixes on top of the channel interrupts reassignment series, both
> addressing race conditions between the initialization of the target
> CPUs and the CPU hot removal path. (Fixes: tags refer to commit IDs
> from the hyperv tree and should be considered as placeholders; please
> let me know how you'd prefer to handle these...)
Thanks for the heads-up. I will change the tags if I end up rebasing
hyperv-next.
Wei.
>
> Thanks,
> Andrea
>
> Andrea Parri (Microsoft) (2):
> Drivers: hv: vmbus: Resolve race between init_vp_index() and CPU
> hotplug
> Drivers: hv: vmbus: Resolve more races involving init_vp_index()
>
> drivers/hv/channel_mgmt.c | 66 +++++++++++++++++++--------------------
> drivers/hv/hyperv_vmbus.h | 48 ++++++++++++++++++++++++++++
> drivers/hv/vmbus_drv.c | 19 +++++++----
> include/linux/hyperv.h | 7 +++++
> 4 files changed, 101 insertions(+), 39 deletions(-)
>
> --
> 2.25.1
>
^ permalink raw reply
* [PATCH] PCI: hv: Use struct_size() helper
From: Gustavo A. R. Silva @ 2020-05-25 16:43 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas
Cc: linux-hyperv, linux-pci, linux-kernel, Gustavo A. R. Silva,
Kees Cook
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct hv_dr_state {
...
struct hv_pcidev_description func[];
};
struct pci_bus_relations {
...
struct pci_function_description func[];
} __packed;
Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes.
So, replace the following forms:
offsetof(struct hv_dr_state, func) +
(sizeof(struct hv_pcidev_description) *
(relations->device_count))
offsetof(struct pci_bus_relations, func) +
(sizeof(struct pci_function_description) *
(bus_rel->device_count))
with:
struct_size(dr, func, relations->device_count)
and
struct_size(bus_rel, func, bus_rel->device_count)
respectively.
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
drivers/pci/controller/pci-hyperv.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 892f3a742117a..bf40ff09c99d6 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -2213,10 +2213,8 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
struct hv_dr_state *dr;
int i;
- dr = kzalloc(offsetof(struct hv_dr_state, func) +
- (sizeof(struct hv_pcidev_description) *
- (relations->device_count)), GFP_NOWAIT);
-
+ dr = kzalloc(struct_size(dr, func, relations->device_count),
+ GFP_NOWAIT);
if (!dr)
return;
@@ -2250,10 +2248,8 @@ static void hv_pci_devices_present2(struct hv_pcibus_device *hbus,
struct hv_dr_state *dr;
int i;
- dr = kzalloc(offsetof(struct hv_dr_state, func) +
- (sizeof(struct hv_pcidev_description) *
- (relations->device_count)), GFP_NOWAIT);
-
+ dr = kzalloc(struct_size(dr, func, relations->device_count),
+ GFP_NOWAIT);
if (!dr)
return;
@@ -2447,9 +2443,8 @@ static void hv_pci_onchannelcallback(void *context)
bus_rel = (struct pci_bus_relations *)buffer;
if (bytes_recvd <
- offsetof(struct pci_bus_relations, func) +
- (sizeof(struct pci_function_description) *
- (bus_rel->device_count))) {
+ struct_size(bus_rel, func,
+ bus_rel->device_count)) {
dev_err(&hbus->hdev->device,
"bus relations too small\n");
break;
@@ -2462,9 +2457,8 @@ static void hv_pci_onchannelcallback(void *context)
bus_rel2 = (struct pci_bus_relations2 *)buffer;
if (bytes_recvd <
- offsetof(struct pci_bus_relations2, func) +
- (sizeof(struct pci_function_description2) *
- (bus_rel2->device_count))) {
+ struct_size(bus_rel2, func,
+ bus_rel2->device_count)) {
dev_err(&hbus->hdev->device,
"bus relations v2 too small\n");
break;
--
2.26.2
^ permalink raw reply related
* [PATCH 0/2] drivers/iommu: Constify structs
From: Rikard Falkeborn @ 2020-05-25 21:49 UTC (permalink / raw)
To: Joerg Roedel
Cc: iommu, linux-kernel, Rikard Falkeborn, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Wei Liu, linux-hyperv,
Maxime Ripard, Chen-Yu Tsai, linux-arm-kernel
Constify some structs with function pointers to allow the compiler to
put them in read-only memory. There is not dependency between the
patches.
Rikard Falkeborn (2):
iommu/hyper-v: Constify hyperv_ir_domain_ops
iommu/sun50i: Constify sun50i_iommu_ops
drivers/iommu/hyperv-iommu.c | 2 +-
drivers/iommu/sun50i-iommu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.26.2
^ permalink raw reply
* [PATCH 1/2] iommu/hyper-v: Constify hyperv_ir_domain_ops
From: Rikard Falkeborn @ 2020-05-25 21:49 UTC (permalink / raw)
To: Joerg Roedel
Cc: iommu, linux-kernel, Rikard Falkeborn, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Wei Liu, linux-hyperv
In-Reply-To: <20200525214958.30015-1-rikard.falkeborn@gmail.com>
The struct hyperv_ir_domain_ops is not modified and can be made const to
allow the compiler to put it in read-only memory.
Before:
text data bss dec hex filename
2916 1180 1120 5216 1460 drivers/iommu/hyperv-iommu.o
After:
text data bss dec hex filename
3044 1052 1120 5216 1460 drivers/iommu/hyperv-iommu.o
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
drivers/iommu/hyperv-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/hyperv-iommu.c b/drivers/iommu/hyperv-iommu.c
index a386b83e0e34..3c0c67a99c7b 100644
--- a/drivers/iommu/hyperv-iommu.c
+++ b/drivers/iommu/hyperv-iommu.c
@@ -131,7 +131,7 @@ static int hyperv_irq_remapping_activate(struct irq_domain *domain,
return 0;
}
-static struct irq_domain_ops hyperv_ir_domain_ops = {
+static const struct irq_domain_ops hyperv_ir_domain_ops = {
.alloc = hyperv_irq_remapping_alloc,
.free = hyperv_irq_remapping_free,
.activate = hyperv_irq_remapping_activate,
--
2.26.2
^ permalink raw reply related
* Re: [PATCH 1/2] iommu/hyper-v: Constify hyperv_ir_domain_ops
From: Wei Liu @ 2020-05-26 9:09 UTC (permalink / raw)
To: Rikard Falkeborn
Cc: Joerg Roedel, iommu, linux-kernel, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Wei Liu, linux-hyperv
In-Reply-To: <20200525214958.30015-2-rikard.falkeborn@gmail.com>
On Mon, May 25, 2020 at 11:49:57PM +0200, Rikard Falkeborn wrote:
> The struct hyperv_ir_domain_ops is not modified and can be made const to
> allow the compiler to put it in read-only memory.
>
> Before:
> text data bss dec hex filename
> 2916 1180 1120 5216 1460 drivers/iommu/hyperv-iommu.o
>
> After:
> text data bss dec hex filename
> 3044 1052 1120 5216 1460 drivers/iommu/hyperv-iommu.o
>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
Acked-by: Wei Liu <wei.liu@kernel.org>
> ---
> drivers/iommu/hyperv-iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/hyperv-iommu.c b/drivers/iommu/hyperv-iommu.c
> index a386b83e0e34..3c0c67a99c7b 100644
> --- a/drivers/iommu/hyperv-iommu.c
> +++ b/drivers/iommu/hyperv-iommu.c
> @@ -131,7 +131,7 @@ static int hyperv_irq_remapping_activate(struct irq_domain *domain,
> return 0;
> }
>
> -static struct irq_domain_ops hyperv_ir_domain_ops = {
> +static const struct irq_domain_ops hyperv_ir_domain_ops = {
> .alloc = hyperv_irq_remapping_alloc,
> .free = hyperv_irq_remapping_free,
> .activate = hyperv_irq_remapping_activate,
> --
> 2.26.2
>
^ permalink raw reply
* Re: [PATCH] PCI: hv: Use struct_size() helper
From: Wei Liu @ 2020-05-26 9:38 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas, linux-hyperv,
linux-pci, linux-kernel, Gustavo A. R. Silva, Kees Cook
In-Reply-To: <20200525164319.GA13596@embeddedor>
On Mon, May 25, 2020 at 11:43:19AM -0500, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct hv_dr_state {
> ...
> struct hv_pcidev_description func[];
> };
>
> struct pci_bus_relations {
> ...
> struct pci_function_description func[];
> } __packed;
>
> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes.
>
> So, replace the following forms:
>
> offsetof(struct hv_dr_state, func) +
> (sizeof(struct hv_pcidev_description) *
> (relations->device_count))
>
> offsetof(struct pci_bus_relations, func) +
> (sizeof(struct pci_function_description) *
> (bus_rel->device_count))
>
> with:
>
> struct_size(dr, func, relations->device_count)
>
> and
>
> struct_size(bus_rel, func, bus_rel->device_count)
>
> respectively.
>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Wei Liu <wei.liu@kernel.org>
FAOD I expect this patch to go through pci tree.
^ permalink raw reply
* [RFC PATCH 0/2] VMBus channel interrupts re-balancing
From: Andrea Parri (Microsoft) @ 2020-05-26 22:32 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Nuno Das Neves, Andrea Parri (Microsoft)
The RFC introduces constructs to re-balance the channel interrupts at
CPU hotplug and at device hotplug operations, the latter being indeed
"closure/opening operations" to enable the re-balancing also in cases
when the device is just being closed/re-opened (as in "ethtool -L").
These changes originated from (and address /try to resolve) two known
limitations of the current interrupts-to-CPUs mapping scheme, that is,
(1) the "static" nature of this mapping scheme (that, e.g., can end up
preventing the hot removal of certain CPUs) and (2) the lack of global
visibility in such scheme (where devices/channels are mapped only "one
at a time"/as they are offered, with the end result that globally the
various interrupts are not always evenly spread across CPUs).
Andrea Parri (Microsoft) (2):
Drivers: hv: vmbus: Re-balance channel interrupts across CPUs at CPU
hotplug
Drivers: hv: vmbus: Re-balance channel interrupts across CPUs at
device hotplug
drivers/hv/channel.c | 81 ++++++++++++
drivers/hv/channel_mgmt.c | 263 ++++++++++++++++++++++++++++++++++++++
drivers/hv/connection.c | 32 +++--
drivers/hv/hv.c | 62 +++++----
drivers/hv/hyperv_vmbus.h | 78 +++++++++++
drivers/hv/vmbus_drv.c | 45 ++-----
include/linux/hyperv.h | 26 ++++
kernel/cpu.c | 1 +
8 files changed, 523 insertions(+), 65 deletions(-)
--
2.25.1
^ permalink raw reply
* [RFC PATCH 1/2] Drivers: hv: vmbus: Re-balance channel interrupts across CPUs at CPU hotplug
From: Andrea Parri (Microsoft) @ 2020-05-26 22:32 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Nuno Das Neves, Andrea Parri (Microsoft)
In-Reply-To: <20200526223218.184057-1-parri.andrea@gmail.com>
CPU hot removals and additions present an opportunity for (re-)balancing
the channel interrupts across the available CPUs. Current code does not
balance the interrupts at CPU hotplug; furthermore/consequently, the hot
removal path currently fails (to remove the specified CPU) whenever some
interrupt is bound to the CPU to be removed and the VMBus is connected.
Address such issues by implementing vmbus_balance_vp_indexes_at_cpuhp():
invoke this primitive to balance the channel interrupts across available
CPUs at CPU hotplug operations. In the hot removal path, such primitive
will (try to) move/balance interrupts out of the to-be-removed CPU so as
to meet the user request to hot remove the CPU.
The balancing algorithm distributes the channel interrupts evenly across
the available CPUs and NUMA nodes; to do so, it introduces and maintains
per-device and per-connection channel statistics/counts to keep track of
the (current) assignments of the channels to the CPUs/nodes. By design,
only "performance"-critical channels/devices are "balanced".
The proposed algorithm relies on the (recently introduced) capability to
reassign a channel interrupt to a CPU (cf., the CHANNELMSG_MODIFYCHANNEL
message type). As such, the new balancing process is effective starting
with VMBus version 4.1 (no changes in semantics or behavior are intended
for VMBus versions lower than 4.1).
Suggested-by: Nuno Das Neves <nuno.das@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel.c | 38 +++++++
drivers/hv/channel_mgmt.c | 219 ++++++++++++++++++++++++++++++++++++++
drivers/hv/connection.c | 23 ++--
drivers/hv/hv.c | 62 ++++++-----
drivers/hv/hyperv_vmbus.h | 72 +++++++++++++
drivers/hv/vmbus_drv.c | 45 +++-----
include/linux/hyperv.h | 22 +++-
kernel/cpu.c | 1 +
8 files changed, 416 insertions(+), 66 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 90070b337c10d..2974aa9dc956c 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -18,6 +18,7 @@
#include <linux/uio.h>
#include <linux/interrupt.h>
#include <asm/page.h>
+#include <asm/mshyperv.h>
#include "hyperv_vmbus.h"
@@ -317,6 +318,43 @@ int vmbus_send_modifychannel(u32 child_relid, u32 target_vp)
}
EXPORT_SYMBOL_GPL(vmbus_send_modifychannel);
+bool vmbus_modifychannel(struct vmbus_channel *channel,
+ u32 origin_cpu, u32 target_cpu)
+{
+ if (vmbus_send_modifychannel(channel->offermsg.child_relid,
+ hv_cpu_number_to_vp_number(target_cpu)))
+ return false;
+
+ /*
+ * Warning. At this point, there is *no* guarantee that the host will
+ * have successfully processed the vmbus_send_modifychannel() request.
+ * See the header comment of vmbus_send_modifychannel() for more info.
+ *
+ * Lags in the processing of the above vmbus_send_modifychannel() can
+ * result in missed interrupts if the "old" target CPU is taken offline
+ * before Hyper-V starts sending interrupts to the "new" target CPU.
+ * But apart from this offlining scenario, the code tolerates such
+ * lags. It will function correctly even if a channel interrupt comes
+ * in on a CPU that is different from the channel target_cpu value.
+ */
+
+ channel->target_cpu = target_cpu;
+ channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
+ channel->numa_node = cpu_to_node(target_cpu);
+
+ /* See init_vp_index(). */
+ if (hv_is_perf_channel(channel))
+ hv_update_alloced_cpus(origin_cpu, target_cpu);
+
+ /* Currently set only for storvsc channels. */
+ if (channel->change_target_cpu_callback) {
+ (*channel->change_target_cpu_callback)(channel,
+ origin_cpu, target_cpu);
+ }
+
+ return true;
+}
+
/*
* create_gpadl_header - Creates a gpadl for the specified buffer
*/
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 417a95e5094dd..c158f86787940 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -497,10 +497,14 @@ static void vmbus_add_channel_work(struct work_struct *work)
/*
* Start the process of binding the primary channel to the driver
*/
+
+ /* See vmbus_balance_vp_indexes_at_cpuhp(). */
+ mutex_lock(&vmbus_connection.channel_mutex);
newchannel->device_obj = vmbus_device_create(
&newchannel->offermsg.offer.if_type,
&newchannel->offermsg.offer.if_instance,
newchannel);
+ mutex_unlock(&vmbus_connection.channel_mutex);
if (!newchannel->device_obj)
goto err_deq_chan;
@@ -515,6 +519,8 @@ static void vmbus_add_channel_work(struct work_struct *work)
if (ret != 0) {
pr_err("unable to add child device object (relid %d)\n",
newchannel->offermsg.child_relid);
+ if (hv_is_perf_channel(newchannel))
+ free_chn_counts(&newchannel->device_obj->chn_cnt);
kfree(newchannel->device_obj);
goto err_deq_chan;
}
@@ -667,6 +673,219 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
queue_work(wq, &newchannel->add_channel_work);
}
+static void filter_on_cpus(cpumask_var_t cpu_msk,
+ struct vmbus_channel_counts *chn_cnt)
+{
+ unsigned int min_cnt = MAX_CHANNEL_RELIDS;
+ unsigned int cpu;
+
+ for_each_cpu(cpu, cpu_msk) {
+ unsigned int cnt = chn_cnt->cpu[cpu];
+
+ if (cnt < min_cnt)
+ min_cnt = cnt;
+ }
+
+ for_each_cpu(cpu, cpu_msk) {
+ if (chn_cnt->cpu[cpu] > min_cnt)
+ cpumask_clear_cpu(cpu, cpu_msk);
+ }
+}
+
+static void filter_on_nodes(cpumask_var_t cpu_msk,
+ struct vmbus_channel_counts *chn_cnt)
+{
+ unsigned int min_cnt = MAX_CHANNEL_RELIDS;
+ unsigned int cpu;
+
+ for_each_cpu(cpu, cpu_msk) {
+ unsigned int node = cpu_to_node(cpu);
+ unsigned int cnt = chn_cnt->node[node];
+
+ if (cnt < min_cnt)
+ min_cnt = cnt;
+
+ cpu = cpumask_last(cpumask_of_node(node));
+ }
+
+ for_each_cpu(cpu, cpu_msk) {
+ unsigned int node = cpu_to_node(cpu);
+ const struct cpumask *msk_node = cpumask_of_node(node);
+
+ if (chn_cnt->node[node] > min_cnt)
+ cpumask_andnot(cpu_msk, cpu_msk, msk_node);
+
+ cpu = cpumask_last(msk_node);
+ }
+}
+
+static inline void __filter_vp_index(struct vmbus_channel_counts *chn_cnt,
+ cpumask_var_t cpu_msk)
+{
+ filter_on_cpus(cpu_msk, chn_cnt);
+ filter_on_nodes(cpu_msk, chn_cnt);
+}
+
+static void filter_vp_index(struct hv_device *hv_dev, cpumask_var_t cpu_msk)
+{
+ /*
+ * The selection of the target CPUs is performed in two domains,
+ * the device domain and the connection domain. At each domain,
+ * in turn, invalid CPUs are filtered out at two levels, the CPU
+ * level and the NUMA-node level: CPUs corresponding to channel
+ * counts *greater* than the minimum channel count for the given
+ * level/domain are cleared in the mask of candidate target CPUs.
+ */
+
+ __filter_vp_index(&hv_dev->chn_cnt, cpu_msk);
+ __filter_vp_index(&vmbus_connection.chn_cnt, cpu_msk);
+}
+
+static void balance_vp_index(struct vmbus_channel *chn,
+ struct hv_device *hv_dev, cpumask_var_t cpu_msk)
+{
+ u32 cur_cpu = chn->target_cpu, tgt_cpu = cur_cpu;
+
+ if (chn->state != CHANNEL_OPENED_STATE) {
+ /*
+ * The channel may never have been opened or it may be in
+ * a closed/closing state; if so, do not touch the target
+ * CPU of this channel.
+ */
+ goto update_chn_cnts;
+ }
+
+ /*
+ * The channel was open, and it will remain open until we release
+ * channel_mutex, cf. the use of channel_mutex and channel->state
+ * in vmbus_disconnect_ring() -> vmbus_close_internal().
+ */
+
+ if (!hv_is_perf_channel(chn)) {
+ /*
+ * Only used by the CPU hot removal path, remark that
+ * the connect CPU can not go offline.
+ */
+ tgt_cpu = VMBUS_CONNECT_CPU;
+ goto modify_vp_index;
+ }
+
+ /*
+ * Retrieve the new "candidate" target CPUs for this channel
+ * /device; see the inline comments in filter_vp_index() for
+ * a high-level description of this algorithm.
+ */
+ filter_vp_index(hv_dev, cpu_msk);
+
+ /*
+ * (Try to) preserve the channel's CPU and NUMA node affinities
+ * respectively:
+ */
+
+ if (cpumask_test_cpu(cur_cpu, cpu_msk))
+ goto update_chn_cnts;
+
+ /*
+ * If we reach here, a modification of the channel's target CPU
+ * is "needed".
+ */
+
+ tgt_cpu = cpumask_first_and(cpumask_of_node(cpu_to_node(cur_cpu)),
+ cpu_msk);
+ if (tgt_cpu < nr_cpu_ids)
+ goto modify_vp_index;
+
+ /*
+ * It was not possible to preserve this channel's CPU and NUMA
+ * node affinities; pick the "first" candidate target CPU (the
+ * CPU mask can not be empty).
+ */
+ tgt_cpu = cpumask_first(cpu_msk);
+
+modify_vp_index:
+ if (!vmbus_modifychannel(chn, cur_cpu, tgt_cpu))
+ tgt_cpu = cur_cpu;
+
+update_chn_cnts:
+ /* Do not account for non-"perf" channels in chn_cnt. */
+ if (!hv_is_perf_channel(chn))
+ return;
+
+ inc_chn_counts(&hv_dev->chn_cnt, tgt_cpu);
+ inc_chn_counts(&vmbus_connection.chn_cnt, tgt_cpu);
+}
+
+void vmbus_balance_vp_indexes_at_cpuhp(unsigned int cpu, bool add)
+{
+ struct vmbus_channel *chn, *sc;
+ cpumask_var_t cpu_msk;
+
+ lockdep_assert_cpus_held();
+ lockdep_assert_held(&vmbus_connection.channel_mutex);
+
+ WARN_ON(!cpumask_test_cpu(cpu, cpu_online_mask));
+
+ /* See the header comment for vmbus_send_modifychannel(). */
+ if (vmbus_proto_version < VERSION_WIN10_V4_1)
+ return;
+
+ if (!alloc_cpumask_var(&cpu_msk, GFP_KERNEL))
+ return;
+
+ reset_chn_counts(&vmbus_connection.chn_cnt);
+
+ list_for_each_entry(chn, &vmbus_connection.chn_list, listentry) {
+ struct hv_device *dev = chn->device_obj;
+
+ /*
+ * The device may not have been allocated/assigned to
+ * the primary channel yet; if so, do not balance the
+ * channels associated to this device. If dev != NULL,
+ * the synchronization on channel_mutex ensures that
+ * the device's chn_cnt has been (properly) allocated
+ * *and* initialized, cf. vmbus_add_channel_work().
+ */
+ if (dev == NULL)
+ continue;
+
+ /*
+ * By design, non-"perf" channels do not take part in
+ * the balancing process.
+ *
+ * The user may have assigned some non-"perf" channel
+ * to this CPU. To satisfy the user's request to hot
+ * remove the CPU, we will re-assign such channels to
+ * the connect CPU; cf. balance_vp_index().
+ */
+ if (!hv_is_perf_channel(chn)) {
+ if (add)
+ continue;
+ /*
+ * Assume that the non-"perf" channel has no
+ * sub-channels.
+ */
+ if (chn->target_cpu != cpu)
+ continue;
+ } else {
+ reset_chn_counts(&dev->chn_cnt);
+ }
+
+ cpumask_copy(cpu_msk, cpu_online_mask);
+ if (!add)
+ cpumask_clear_cpu(cpu, cpu_msk);
+ balance_vp_index(chn, dev, cpu_msk);
+
+ list_for_each_entry(sc, &chn->sc_list, sc_list) {
+ cpumask_copy(cpu_msk, cpu_online_mask);
+ if (!add)
+ cpumask_clear_cpu(cpu, cpu_msk);
+ balance_vp_index(sc, dev, cpu_msk);
+ }
+ }
+
+ free_cpumask_var(cpu_msk);
+}
+
/*
* We use this state to statically distribute the channel interrupt load.
*/
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 11170d9a2e1a5..7ec562fac8e58 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -183,6 +183,18 @@ int vmbus_connect(void)
spin_lock_init(&vmbus_connection.channelmsg_lock);
INIT_LIST_HEAD(&vmbus_connection.chn_list);
+
+ vmbus_connection.channels = kcalloc(MAX_CHANNEL_RELIDS,
+ sizeof(struct vmbus_channel *),
+ GFP_KERNEL);
+ if (vmbus_connection.channels == NULL) {
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ if (alloc_chn_counts(&vmbus_connection.chn_cnt))
+ goto cleanup;
+
mutex_init(&vmbus_connection.channel_mutex);
/*
@@ -248,14 +260,6 @@ int vmbus_connect(void)
pr_info("Vmbus version:%d.%d\n",
version >> 16, version & 0xFFFF);
- vmbus_connection.channels = kcalloc(MAX_CHANNEL_RELIDS,
- sizeof(struct vmbus_channel *),
- GFP_KERNEL);
- if (vmbus_connection.channels == NULL) {
- ret = -ENOMEM;
- goto cleanup;
- }
-
kfree(msginfo);
return 0;
@@ -295,6 +299,9 @@ void vmbus_disconnect(void)
hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[1]);
vmbus_connection.monitor_pages[0] = NULL;
vmbus_connection.monitor_pages[1] = NULL;
+
+ free_chn_counts(&vmbus_connection.chn_cnt);
+ kfree(vmbus_connection.channels);
}
/*
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 188b42b07f07b..e1cade3c3c967 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -198,6 +198,20 @@ void hv_synic_enable_regs(unsigned int cpu)
int hv_synic_init(unsigned int cpu)
{
+ /*
+ * The CPU has been hot added: try to re-balance the channels
+ * across the online CPUs (including "this" CPU), provided that
+ * the VMBus is connected; in part., avoid the re-balancing at
+ * the very first CPU initialization.
+ *
+ * See also inline comments in hv_synic_cleanup().
+ */
+ if (vmbus_connection.conn_state == CONNECTED) {
+ mutex_lock(&vmbus_connection.channel_mutex);
+ vmbus_balance_vp_indexes_at_cpuhp(cpu, true);
+ mutex_unlock(&vmbus_connection.channel_mutex);
+ }
+
hv_synic_enable_regs(cpu);
hv_stimer_legacy_init(cpu, VMBUS_MESSAGE_SINT);
@@ -243,10 +257,6 @@ void hv_synic_disable_regs(unsigned int cpu)
int hv_synic_cleanup(unsigned int cpu)
{
- struct vmbus_channel *channel, *sc;
- bool channel_found = false;
- unsigned long flags;
-
/*
* Hyper-V does not provide a way to change the connect CPU once
* it is set; we must prevent the connect CPU from going offline.
@@ -254,35 +264,37 @@ int hv_synic_cleanup(unsigned int cpu)
if (cpu == VMBUS_CONNECT_CPU)
return -EBUSY;
+ mutex_lock(&vmbus_connection.channel_mutex);
+ /*
+ * The CPU is being hot removed: re-balance the channels across
+ * the online CPUs but excluding "this" CPU.
+ *
+ * Note. We do not roll back on failure; IOW, we may end up with
+ * situations where the (re-)balancing process failed but some of
+ * the channels have been re-assigned to different CPUs.
+ *
+ * Also, the balancing process makes no attempt to re-assign "non-
+ * open" channels, that is, it only applies to channels which are
+ * found in CHANNEL_OPENED_STATE.
+ */
+ vmbus_balance_vp_indexes_at_cpuhp(cpu, false);
+
/*
* Search for channels which are bound to the CPU we're about to
* cleanup. In case we find one and vmbus is still connected, we
* fail; this will effectively prevent CPU offlining.
- *
- * TODO: Re-bind the channels to different CPUs.
*/
- mutex_lock(&vmbus_connection.channel_mutex);
- list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
- if (channel->target_cpu == cpu) {
- channel_found = true;
- break;
- }
- spin_lock_irqsave(&channel->lock, flags);
- list_for_each_entry(sc, &channel->sc_list, sc_list) {
- if (sc->target_cpu == cpu) {
- channel_found = true;
- break;
- }
- }
- spin_unlock_irqrestore(&channel->lock, flags);
- if (channel_found)
- break;
+ if (hv_is_busy_cpu(cpu) && vmbus_connection.conn_state == CONNECTED) {
+ /*
+ * The CPU was busy, and we are going to prevent it from
+ * going offline; balance the channels accordingly.
+ */
+ vmbus_balance_vp_indexes_at_cpuhp(cpu, true);
+ mutex_unlock(&vmbus_connection.channel_mutex);
+ return -EBUSY;
}
mutex_unlock(&vmbus_connection.channel_mutex);
- if (channel_found && vmbus_connection.conn_state == CONNECTED)
- return -EBUSY;
-
hv_stimer_legacy_cleanup(cpu);
hv_synic_disable_regs(cpu);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 40e2b9f91163c..b6d194caf69ed 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -18,6 +18,7 @@
#include <linux/atomic.h>
#include <linux/hyperv.h>
#include <linux/interrupt.h>
+#include <linux/slab.h>
#include "hv_trace.h"
@@ -250,6 +251,19 @@ struct vmbus_connection {
/* Array of channels */
struct vmbus_channel **channels;
+ /*
+ * Channel counts used by the channel-interrupts balancing scheme:
+ *
+ * - chn_cnt.cpu[cpu], the number of channel interrupts assigned
+ * to CPU #cpu;
+ *
+ * - chn_cnt.node[node], the number of channel interrupts assigned
+ * to NUMA node #node.
+ *
+ * The counts refer to "open" *and "performance" channels only.
+ */
+ struct vmbus_channel_counts chn_cnt;
+
/*
* An offer message is handled first on the work_queue, and then
* is further handled on handle_primary_chan_wq or
@@ -344,6 +358,8 @@ struct vmbus_channel *relid2channel(u32 relid);
void vmbus_free_channels(void);
+void vmbus_balance_vp_indexes_at_cpuhp(unsigned int cpu, bool add);
+
/* Connection interface */
int vmbus_connect(void);
@@ -443,6 +459,62 @@ static inline void hv_update_alloced_cpus(unsigned int old_cpu,
hv_clear_alloced_cpu(old_cpu);
}
+static inline bool hv_is_busy_cpu(unsigned int cpu)
+{
+ struct vmbus_channel *channel, *sc;
+
+ lockdep_assert_held(&vmbus_connection.channel_mutex);
+ /*
+ * List additions/deletions as well as updates of the target CPUs are
+ * protected by channel_mutex.
+ */
+ list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
+ if (channel->target_cpu == cpu)
+ return true;
+ list_for_each_entry(sc, &channel->sc_list, sc_list) {
+ if (sc->target_cpu == cpu)
+ return true;
+ }
+ }
+ return false;
+}
+
+static inline int alloc_chn_counts(struct vmbus_channel_counts *chn_cnt)
+{
+ chn_cnt->cpu = kcalloc(nr_cpu_ids, sizeof(unsigned int), GFP_KERNEL);
+ if (chn_cnt->cpu == NULL)
+ return -ENOMEM;
+
+ chn_cnt->node = kcalloc(nr_node_ids, sizeof(unsigned int), GFP_KERNEL);
+ if (chn_cnt->node == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static inline void inc_chn_counts(struct vmbus_channel_counts *chn_cnt,
+ unsigned int cpu)
+{
+ chn_cnt->cpu[cpu] += 1;
+ chn_cnt->node[cpu_to_node(cpu)] += 1;
+}
+
+static inline void reset_chn_counts(struct vmbus_channel_counts *chn_cnt)
+{
+ unsigned int i;
+
+ for_each_online_cpu(i)
+ chn_cnt->cpu[i] = 0;
+ for_each_online_node(i)
+ chn_cnt->node[i] = 0;
+}
+
+static inline void free_chn_counts(struct vmbus_channel_counts *chn_cnt)
+{
+ kfree(chn_cnt->cpu);
+ kfree(chn_cnt->node);
+}
+
#ifdef CONFIG_HYPERV_TESTING
int hv_debug_add_dev_dir(struct hv_device *dev);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 47747755d2e1d..a40b930fb86a5 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -980,6 +980,9 @@ static void vmbus_device_release(struct device *device)
mutex_lock(&vmbus_connection.channel_mutex);
hv_process_channel_removal(channel);
mutex_unlock(&vmbus_connection.channel_mutex);
+
+ if (hv_is_perf_channel(channel))
+ free_chn_counts(&hv_dev->chn_cnt);
kfree(hv_dev);
}
@@ -1745,38 +1748,8 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
if (target_cpu == origin_cpu)
goto cpu_store_unlock;
- if (vmbus_send_modifychannel(channel->offermsg.child_relid,
- hv_cpu_number_to_vp_number(target_cpu))) {
+ if (!vmbus_modifychannel(channel, origin_cpu, target_cpu))
ret = -EIO;
- goto cpu_store_unlock;
- }
-
- /*
- * Warning. At this point, there is *no* guarantee that the host will
- * have successfully processed the vmbus_send_modifychannel() request.
- * See the header comment of vmbus_send_modifychannel() for more info.
- *
- * Lags in the processing of the above vmbus_send_modifychannel() can
- * result in missed interrupts if the "old" target CPU is taken offline
- * before Hyper-V starts sending interrupts to the "new" target CPU.
- * But apart from this offlining scenario, the code tolerates such
- * lags. It will function correctly even if a channel interrupt comes
- * in on a CPU that is different from the channel target_cpu value.
- */
-
- channel->target_cpu = target_cpu;
- channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
- channel->numa_node = cpu_to_node(target_cpu);
-
- /* See init_vp_index(). */
- if (hv_is_perf_channel(channel))
- hv_update_alloced_cpus(origin_cpu, target_cpu);
-
- /* Currently set only for storvsc channels. */
- if (channel->change_target_cpu_callback) {
- (*channel->change_target_cpu_callback)(channel,
- origin_cpu, target_cpu);
- }
cpu_store_unlock:
mutex_unlock(&vmbus_connection.channel_mutex);
@@ -1972,6 +1945,15 @@ struct hv_device *vmbus_device_create(const guid_t *type,
guid_copy(&child_device_obj->dev_instance, instance);
child_device_obj->vendor_id = 0x1414; /* MSFT vendor ID */
+ if (!hv_is_perf_channel(channel))
+ return child_device_obj;
+
+ if (alloc_chn_counts(&child_device_obj->chn_cnt)) {
+ free_chn_counts(&child_device_obj->chn_cnt);
+ kfree(child_device_obj);
+ return NULL;
+ }
+
return child_device_obj;
}
@@ -2616,7 +2598,6 @@ static void __exit vmbus_exit(void)
hv_debug_rm_all_dir();
vmbus_free_channels();
- kfree(vmbus_connection.channels);
if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
kmsg_dump_unregister(&hv_kmsg_dumper);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 40df3103e890b..0e9f695ea8f87 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1175,6 +1175,12 @@ struct hv_driver {
};
+/* For channel-interrupts balancing. */
+struct vmbus_channel_counts {
+ unsigned int *cpu;
+ unsigned int *node;
+};
+
/* Base device object */
struct hv_device {
/* the device type id of this device */
@@ -1191,9 +1197,21 @@ struct hv_device {
struct vmbus_channel *channel;
struct kset *channels_kset;
+ /*
+ * Channel counts used by the channel-interrupts balancing scheme:
+ *
+ * - chn_cnt.cpu[cpu], the number of channel interrupts of this
+ * device assigned to CPU #cpu;
+ *
+ * - chn_cnt.node[node], the number of channel interrupts of this
+ * device assigned to NUMA node #node.
+ *
+ * The counts refer to "open" *and "performance" channels only.
+ */
+ struct vmbus_channel_counts chn_cnt;
+
/* place holder to keep track of the dir for hv device in debugfs */
struct dentry *debug_dir;
-
};
@@ -1523,6 +1541,8 @@ extern __u32 vmbus_proto_version;
int vmbus_send_tl_connect_request(const guid_t *shv_guest_servie_id,
const guid_t *shv_host_servie_id);
int vmbus_send_modifychannel(u32 child_relid, u32 target_vp);
+bool vmbus_modifychannel(struct vmbus_channel *channel,
+ u32 origin_cpu, u32 target_cpu);
void vmbus_set_event(struct vmbus_channel *channel);
/* Get the start of the ring buffer. */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2371292f30b03..6853b1d3e3ce0 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -328,6 +328,7 @@ void lockdep_assert_cpus_held(void)
percpu_rwsem_assert_held(&cpu_hotplug_lock);
}
+EXPORT_SYMBOL_GPL(lockdep_assert_cpus_held);
static void lockdep_acquire_cpus_lock(void)
{
--
2.25.1
^ permalink raw reply related
* [RFC PATCH 2/2] Drivers: hv: vmbus: Re-balance channel interrupts across CPUs at device hotplug
From: Andrea Parri (Microsoft) @ 2020-05-26 22:32 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Nuno Das Neves, Andrea Parri (Microsoft)
In-Reply-To: <20200526223218.184057-1-parri.andrea@gmail.com>
Device hot removals and additions (closure and opening) also present a
valid opportunity for (re-)balancing the channel interrupts across the
available CPUs. Current code balances interrupts as they are offered,
but it does not modify the interrupts-to-CPUs assignment if interrupts
are "rescinded"/removed from the system. Moreover, in case of channel
/device offers, the interrupt assignments are performed *one at a time*
and without full visibility into other channels/devices; the result is
that, globally, interrupts are sometimes not evenly spread across CPUs.
Introduce the vmbus_balance_vp_indexes_at_devhp() primitive to balance
the channel interrupts across online CPUs at device hotplug operations.
The primitive triggers a "full" balancing of the interrupts across the
online CPUs and NUMA nodes. The balancing process at device addition/
opening is deferred to a delayed work, to give channels of such device
more chances to be opened. As in vmbus_balance_vp_indexes_at_cpuhp(),
the balancing is applied to "performance" channels only, and it relies
on the (new) capability to re-assign a channel interrupt.
Suggested-by: Nuno Das Neves <nuno.das@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel.c | 43 ++++++++++++++++++++++++++++++++
drivers/hv/channel_mgmt.c | 52 ++++++++++++++++++++++++++++++++++++---
drivers/hv/connection.c | 9 +++++++
drivers/hv/hyperv_vmbus.h | 6 +++++
include/linux/hyperv.h | 6 +++++
5 files changed, 112 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 2974aa9dc956c..076f2d68a1efe 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -11,6 +11,7 @@
#include <linux/kernel.h>
#include <linux/sched.h>
#include <linux/wait.h>
+#include <linux/cpu.h>
#include <linux/mm.h>
#include <linux/slab.h>
#include <linux/module.h>
@@ -112,6 +113,19 @@ int vmbus_alloc_ring(struct vmbus_channel *newchannel,
}
EXPORT_SYMBOL_GPL(vmbus_alloc_ring);
+static void vmbus_add_device_work(struct work_struct *work)
+{
+ struct hv_device *hv_dev =
+ container_of((struct delayed_work *)work, struct hv_device,
+ add_device_work);
+
+ cpus_read_lock();
+ mutex_lock(&vmbus_connection.channel_mutex);
+ vmbus_balance_vp_indexes_at_devhp(hv_dev, true);
+ mutex_unlock(&vmbus_connection.channel_mutex);
+ cpus_read_unlock();
+}
+
static int __vmbus_open(struct vmbus_channel *newchannel,
void *userdata, u32 userdatalen,
void (*onchannelcallback)(void *context), void *context)
@@ -217,6 +231,24 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
}
newchannel->state = CHANNEL_OPENED_STATE;
+ /*
+ * If this is a primary channel and a "perf" channel, queue the
+ * add_device_work to balance the channels of the device across
+ * the online CPUs; the queueing delay should be greater than
+ * the "typical" time required to open such channels, since the
+ * balancing will only apply to *open* channels. The closure
+ * path will wait for our work to complete and start a new re-
+ * balancing, cf. vmbus_disconnect_ring().
+ */
+ if (newchannel->primary_channel == NULL &&
+ hv_is_perf_channel(newchannel)) {
+ struct delayed_work *dwork =
+ &newchannel->device_obj->add_device_work;
+
+ INIT_DELAYED_WORK(dwork, vmbus_add_device_work);
+ queue_delayed_work(vmbus_connection.handle_dev_wq, dwork,
+ 8 * HZ);
+ }
kfree(open_info);
return 0;
@@ -750,6 +782,8 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
/* disconnect ring - close all channels */
int vmbus_disconnect_ring(struct vmbus_channel *channel)
{
+ struct hv_device *hv_dev = channel->device_obj;
+ bool perf_chn = hv_is_perf_channel(channel);
struct vmbus_channel *cur_channel, *tmp;
int ret;
@@ -773,9 +807,18 @@ int vmbus_disconnect_ring(struct vmbus_channel *channel)
/*
* Now close the primary.
*/
+
+ /* See inline comment in __vmbus_open(). */
+ if (perf_chn)
+ cancel_delayed_work_sync(&hv_dev->add_device_work);
+
+ cpus_read_lock();
mutex_lock(&vmbus_connection.channel_mutex);
+ if (perf_chn)
+ vmbus_balance_vp_indexes_at_devhp(hv_dev, false);
ret = vmbus_close_internal(channel);
mutex_unlock(&vmbus_connection.channel_mutex);
+ cpus_read_unlock();
return ret;
}
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c158f86787940..91b1bd2914d79 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -815,6 +815,49 @@ static void balance_vp_index(struct vmbus_channel *chn,
inc_chn_counts(&vmbus_connection.chn_cnt, tgt_cpu);
}
+void vmbus_balance_vp_indexes_at_devhp(struct hv_device *hv_dev, bool add)
+{
+ struct vmbus_channel *chn, *sc;
+ cpumask_var_t cpu_msk;
+
+ lockdep_assert_cpus_held();
+ lockdep_assert_held(&vmbus_connection.channel_mutex);
+
+ WARN_ON(!hv_is_perf_channel(hv_dev->channel));
+
+ /* See the header comment for vmbus_send_modifychannel(). */
+ if (vmbus_proto_version < VERSION_WIN10_V4_1)
+ return;
+
+ if (!alloc_cpumask_var(&cpu_msk, GFP_KERNEL))
+ return;
+
+ reset_chn_counts(&vmbus_connection.chn_cnt);
+
+ list_for_each_entry(chn, &vmbus_connection.chn_list, listentry) {
+ struct hv_device *dev = chn->device_obj;
+
+ /* See comment in vmbus_balance_vp_indexes_at_cpuhp(). */
+ if (!hv_is_perf_channel(chn) || dev == NULL)
+ continue;
+
+ if (dev == hv_dev && !add)
+ continue;
+
+ reset_chn_counts(&dev->chn_cnt);
+
+ cpumask_copy(cpu_msk, cpu_online_mask);
+ balance_vp_index(chn, dev, cpu_msk);
+
+ list_for_each_entry(sc, &chn->sc_list, sc_list) {
+ cpumask_copy(cpu_msk, cpu_online_mask);
+ balance_vp_index(sc, dev, cpu_msk);
+ }
+ }
+
+ free_cpumask_var(cpu_msk);
+}
+
void vmbus_balance_vp_indexes_at_cpuhp(unsigned int cpu, bool add)
{
struct vmbus_channel *chn, *sc;
@@ -840,10 +883,11 @@ void vmbus_balance_vp_indexes_at_cpuhp(unsigned int cpu, bool add)
/*
* The device may not have been allocated/assigned to
* the primary channel yet; if so, do not balance the
- * channels associated to this device. If dev != NULL,
- * the synchronization on channel_mutex ensures that
- * the device's chn_cnt has been (properly) allocated
- * *and* initialized, cf. vmbus_add_channel_work().
+ * channels associated to the device and "defer" this
+ * to vmbus_balance_vp_indexes_at_devhp(). If dev !=
+ * NULL, the synchronization on channel_mutex ensures
+ * that the device's chn_cnt has been allocated *and*
+ * initialized, cf. vmbus_add_channel_work().
*/
if (dev == NULL)
continue;
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 7ec562fac8e58..f93060babd9a4 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -179,6 +179,12 @@ int vmbus_connect(void)
goto cleanup;
}
+ vmbus_connection.handle_dev_wq = create_workqueue("hv_device");
+ if (!vmbus_connection.handle_dev_wq) {
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
INIT_LIST_HEAD(&vmbus_connection.chn_msg_list);
spin_lock_init(&vmbus_connection.channelmsg_lock);
@@ -281,6 +287,9 @@ void vmbus_disconnect(void)
*/
vmbus_initiate_unload(false);
+ if (vmbus_connection.handle_dev_wq)
+ destroy_workqueue(vmbus_connection.handle_dev_wq);
+
if (vmbus_connection.handle_sub_chan_wq)
destroy_workqueue(vmbus_connection.handle_sub_chan_wq);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index b6d194caf69ed..b461cf7efd91c 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -272,6 +272,11 @@ struct vmbus_connection {
struct workqueue_struct *work_queue;
struct workqueue_struct *handle_primary_chan_wq;
struct workqueue_struct *handle_sub_chan_wq;
+ /*
+ * Handling device hotplug/addition work, cf. add_device_work from
+ * struct hv_device.
+ */
+ struct workqueue_struct *handle_dev_wq;
/*
* The number of sub-channels and hv_sock channels that should be
@@ -358,6 +363,7 @@ struct vmbus_channel *relid2channel(u32 relid);
void vmbus_free_channels(void);
+void vmbus_balance_vp_indexes_at_devhp(struct hv_device *hv_dev, bool add);
void vmbus_balance_vp_indexes_at_cpuhp(unsigned int cpu, bool add);
/* Connection interface */
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 0e9f695ea8f87..ce0bf3001792f 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1212,6 +1212,12 @@ struct hv_device {
/* place holder to keep track of the dir for hv device in debugfs */
struct dentry *debug_dir;
+
+ /*
+ * Balance the channel interrupts across the online CPUs at device
+ * hotplug/addition.
+ */
+ struct delayed_work add_device_work;
};
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 0/2] drivers/iommu: Constify structs
From: Joerg Roedel @ 2020-05-27 12:39 UTC (permalink / raw)
To: Rikard Falkeborn
Cc: iommu, linux-kernel, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv, Maxime Ripard,
Chen-Yu Tsai, linux-arm-kernel
In-Reply-To: <20200525214958.30015-1-rikard.falkeborn@gmail.com>
On Mon, May 25, 2020 at 11:49:56PM +0200, Rikard Falkeborn wrote:
> Constify some structs with function pointers to allow the compiler to
> put them in read-only memory. There is not dependency between the
> patches.
>
> Rikard Falkeborn (2):
> iommu/hyper-v: Constify hyperv_ir_domain_ops
> iommu/sun50i: Constify sun50i_iommu_ops
>
> drivers/iommu/hyperv-iommu.c | 2 +-
> drivers/iommu/sun50i-iommu.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Applied both, thanks.
^ permalink raw reply
* Re: [PATCH] PCI: hv: Use struct_size() helper
From: Lorenzo Pieralisi @ 2020-05-28 14:41 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Rob Herring, Bjorn Helgaas, linux-hyperv, linux-pci, linux-kernel,
Gustavo A. R. Silva, Kees Cook
In-Reply-To: <20200525164319.GA13596@embeddedor>
On Mon, May 25, 2020 at 11:43:19AM -0500, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct hv_dr_state {
> ...
> struct hv_pcidev_description func[];
> };
>
> struct pci_bus_relations {
> ...
> struct pci_function_description func[];
> } __packed;
>
> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes.
>
> So, replace the following forms:
>
> offsetof(struct hv_dr_state, func) +
> (sizeof(struct hv_pcidev_description) *
> (relations->device_count))
>
> offsetof(struct pci_bus_relations, func) +
> (sizeof(struct pci_function_description) *
> (bus_rel->device_count))
>
> with:
>
> struct_size(dr, func, relations->device_count)
>
> and
>
> struct_size(bus_rel, func, bus_rel->device_count)
>
> respectively.
>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> drivers/pci/controller/pci-hyperv.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
Applied to pci/hv, thanks.
Lorenzo
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 892f3a742117a..bf40ff09c99d6 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2213,10 +2213,8 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
> struct hv_dr_state *dr;
> int i;
>
> - dr = kzalloc(offsetof(struct hv_dr_state, func) +
> - (sizeof(struct hv_pcidev_description) *
> - (relations->device_count)), GFP_NOWAIT);
> -
> + dr = kzalloc(struct_size(dr, func, relations->device_count),
> + GFP_NOWAIT);
> if (!dr)
> return;
>
> @@ -2250,10 +2248,8 @@ static void hv_pci_devices_present2(struct hv_pcibus_device *hbus,
> struct hv_dr_state *dr;
> int i;
>
> - dr = kzalloc(offsetof(struct hv_dr_state, func) +
> - (sizeof(struct hv_pcidev_description) *
> - (relations->device_count)), GFP_NOWAIT);
> -
> + dr = kzalloc(struct_size(dr, func, relations->device_count),
> + GFP_NOWAIT);
> if (!dr)
> return;
>
> @@ -2447,9 +2443,8 @@ static void hv_pci_onchannelcallback(void *context)
>
> bus_rel = (struct pci_bus_relations *)buffer;
> if (bytes_recvd <
> - offsetof(struct pci_bus_relations, func) +
> - (sizeof(struct pci_function_description) *
> - (bus_rel->device_count))) {
> + struct_size(bus_rel, func,
> + bus_rel->device_count)) {
> dev_err(&hbus->hdev->device,
> "bus relations too small\n");
> break;
> @@ -2462,9 +2457,8 @@ static void hv_pci_onchannelcallback(void *context)
>
> bus_rel2 = (struct pci_bus_relations2 *)buffer;
> if (bytes_recvd <
> - offsetof(struct pci_bus_relations2, func) +
> - (sizeof(struct pci_function_description2) *
> - (bus_rel2->device_count))) {
> + struct_size(bus_rel2, func,
> + bus_rel2->device_count)) {
> dev_err(&hbus->hdev->device,
> "bus relations v2 too small\n");
> break;
> --
> 2.26.2
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox