Re: [PATCH] fbdev/xen-fbfront: Assign fb_info->device
On Tue, Sep 10, 2024 at 09:29:30AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 10.09.24 um 09:22 schrieb Roger Pau Monné:
> > On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote:
> > > From: Jason Andryuk
> > >
> > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in
> > > struct device is NULL since xen-fbfront doesn't assign it and the
> > > memory is kzalloc()-ed. Assign fb_info->device to avoid this.
> > >
> > > This was exposed by the conversion of fb_is_primary_device() to
> > > video_is_primary_device() which dropped a NULL check for struct device.
> > >
> > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers")
> > > Reported-by: Arthur Borsboom
> > > Closes:
> > > https://lore.kernel.org/xen-devel/CALUcmUncX=lkxweisitksdy-coe8qkswhfvccneokfrkd0z...@mail.gmail.com/
> > > Tested-by: Arthur Borsboom
> > > CC: sta...@vger.kernel.org
> > > Signed-off-by: Jason Andryuk
> > Reviewed-by: Roger Pau Monné
> >
> > > ---
> > > The other option would be to re-instate the NULL check in
> > > video_is_primary_device()
> > I do think this is needed, or at least an explanation. The commit
> > message in f178e96de7f0 doesn't mention anything about
> > video_is_primary_device() not allowing being passed a NULL device
> > (like it was possible with fb_is_primary_device()).
> >
> > Otherwise callers of video_is_primary_device() would need to be
> > adjusted to check for device != NULL.
>
> The helper expects a non-NULL pointer. We might want to document this.
A BUG_ON(!dev); might be enough documentation that the function
expected a non-NULL dev IMO.
Thanks, Roger.
Re: [PATCH] fbdev/xen-fbfront: Assign fb_info->device
On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote:
> From: Jason Andryuk
>
> Probing xen-fbfront faults in video_is_primary_device(). The passed-in
> struct device is NULL since xen-fbfront doesn't assign it and the
> memory is kzalloc()-ed. Assign fb_info->device to avoid this.
>
> This was exposed by the conversion of fb_is_primary_device() to
> video_is_primary_device() which dropped a NULL check for struct device.
>
> Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers")
> Reported-by: Arthur Borsboom
> Closes:
> https://lore.kernel.org/xen-devel/CALUcmUncX=lkxweisitksdy-coe8qkswhfvccneokfrkd0z...@mail.gmail.com/
> Tested-by: Arthur Borsboom
> CC: sta...@vger.kernel.org
> Signed-off-by: Jason Andryuk
Reviewed-by: Roger Pau Monné
> ---
> The other option would be to re-instate the NULL check in
> video_is_primary_device()
I do think this is needed, or at least an explanation. The commit
message in f178e96de7f0 doesn't mention anything about
video_is_primary_device() not allowing being passed a NULL device
(like it was possible with fb_is_primary_device()).
Otherwise callers of video_is_primary_device() would need to be
adjusted to check for device != NULL.
Thanks, Roger.
Re: Design session notes: GPU acceleration in Xen
On Mon, Jun 17, 2024 at 08:57:14PM -0400, Demi Marie Obenour wrote: > Given the recent progress on PVH dom0, is it reasonable to assume that > PVH dom0 will be ready in time for R4.3, and that therefore Qubes OS > doesn't need to worry about this problem on x86? PVH dom0 will only be ready (whatever ready means in your use-case) when people test and fix the issues, otherwise it would stay in the same limbo it's currently in. I guess the main blocker for Qubes is the lack of PCI passthrough support in order to test it more aggressively? Thanks, Roger.
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On Thu, Mar 16, 2023 at 04:09:44PM -0700, Stefano Stabellini wrote: > On Thu, 16 Mar 2023, Juergen Gross wrote: > > On 16.03.23 14:53, Alex Deucher wrote: > > > On Thu, Mar 16, 2023 at 9:48 AM Juergen Gross wrote: > > > > > > > > On 16.03.23 14:45, Alex Deucher wrote: > > > > > On Thu, Mar 16, 2023 at 3:50 AM Jan Beulich wrote: > > > > > > > > > > > > On 16.03.2023 00:25, Stefano Stabellini wrote: > > > > > > > On Wed, 15 Mar 2023, Jan Beulich wrote: > > > > > > > > On 15.03.2023 01:52, Stefano Stabellini wrote: > > > > > > > > > On Mon, 13 Mar 2023, Jan Beulich wrote: > > > > > > > > > > On 12.03.2023 13:01, Huang Rui wrote: > > > > > > > > > > > Xen PVH is the paravirtualized mode and takes advantage of > > > > > > > > > > > hardware > > > > > > > > > > > virtualization support when possible. It will using the > > > > > > > > > > > hardware IOMMU > > > > > > > > > > > support instead of xen-swiotlb, so disable swiotlb if > > > > > > > > > > > current domain is > > > > > > > > > > > Xen PVH. > > > > > > > > > > > > > > > > > > > > But the kernel has no way (yet) to drive the IOMMU, so how > > > > > > > > > > can > > > > > > > > > > it get > > > > > > > > > > away without resorting to swiotlb in certain cases (like I/O > > > > > > > > > > to an > > > > > > > > > > address-restricted device)? > > > > > > > > > > > > > > > > > > I think Ray meant that, thanks to the IOMMU setup by Xen, > > > > > > > > > there > > > > > > > > > is no > > > > > > > > > need for swiotlb-xen in Dom0. Address translations are done by > > > > > > > > > the IOMMU > > > > > > > > > so we can use guest physical addresses instead of machine > > > > > > > > > addresses for > > > > > > > > > DMA. This is a similar case to Dom0 on ARM when the IOMMU is > > > > > > > > > available > > > > > > > > > (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the > > > > > > > > > corresponding > > > > > > > > > case is XENFEAT_not_direct_mapped). > > > > > > > > > > > > > > > > But how does Xen using an IOMMU help with, as said, > > > > > > > > address-restricted > > > > > > > > devices? They may still need e.g. a 32-bit address to be > > > > > > > > programmed in, > > > > > > > > and if the kernel has memory beyond the 4G boundary not all I/O > > > > > > > > buffers > > > > > > > > may fulfill this requirement. > > > > > > > > > > > > > > In short, it is going to work as long as Linux has guest physical > > > > > > > addresses (not machine addresses, those could be anything) lower > > > > > > > than > > > > > > > 4GB. > > > > > > > > > > > > > > If the address-restricted device does DMA via an IOMMU, then the > > > > > > > device > > > > > > > gets programmed by Linux using its guest physical addresses (not > > > > > > > machine > > > > > > > addresses). > > > > > > > > > > > > > > The 32-bit restriction would be applied by Linux to its choice of > > > > > > > guest > > > > > > > physical address to use to program the device, the same way it > > > > > > > does > > > > > > > on > > > > > > > native. The device would be fine as it always uses Linux-provided > > > > > > > <4GB > > > > > > > addresses. After the IOMMU translation (pagetable setup by Xen), > > > > > > > we > > > > > > > could get any address, including >4GB addresses, and that is > > > > > > > expected to > > > > > > > work. > > > > > > > > > > > > I understand that's the "normal" way of working. But whatever the > > > > > > swiotlb > > > > > > is used for in baremetal Linux, that would similarly require its use > > > > > > in > > > > > > PVH (or HVM) aiui. So unconditionally disabling it in PVH would look > > > > > > to > > > > > > me like an incomplete attempt to disable its use altogether on x86. > > > > > > What > > > > > > difference of PVH vs baremetal am I missing here? > > > > > > > > > > swiotlb is not usable for GPUs even on bare metal. They often have > > > > > hundreds or megs or even gigs of memory mapped on the device at any > > > > > given time. Also, AMD GPUs support 44-48 bit DMA masks (depending on > > > > > the chip family). > > > > > > > > But the swiotlb isn't per device, but system global. > > > > > > Sure, but if the swiotlb is in use, then you can't really use the GPU. > > > So you get to pick one. > > > > The swiotlb is used only for buffers which are not within the DMA mask of a > > device (see dma_direct_map_page()). So an AMD GPU supporting a 44 bit DMA > > mask > > won't use the swiotlb unless you have a buffer above guest physical address > > of > > 16TB (so basically never). > > > > Disabling swiotlb in such a guest would OTOH mean, that a device with only > > 32 bit DMA mask passed through to this guest couldn't work with buffers > > above 4GB. > > > > I don't think this is acceptable. > > From the Xen subsystem in Linux point of view, the only thing we need to > do is to make sure *not* to enable swiotlb_xen (yes "swiotlb_xen", not > the global swiotlb) on PVH because it is not needed anyway. But this is already the case o
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On Sun, Mar 12, 2023 at 08:01:53PM +0800, Huang Rui wrote:
> Xen PVH is the paravirtualized mode and takes advantage of hardware
> virtualization support when possible. It will using the hardware IOMMU
> support instead of xen-swiotlb, so disable swiotlb if current domain is
> Xen PVH.
>
> Signed-off-by: Huang Rui
> ---
> arch/x86/kernel/pci-dma.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index 30bbe4abb5d6..f5c73dd18f2a 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -74,6 +74,12 @@ static inline void __init pci_swiotlb_detect(void)
> #ifdef CONFIG_SWIOTLB_XEN
> static void __init pci_xen_swiotlb_init(void)
> {
> + /* Xen PVH domain won't use swiotlb */
> + if (xen_pvh_domain()) {
> + x86_swiotlb_enable = false;
> + return;
> + }
I'm very confused by this: pci_xen_swiotlb_init() is only called for
PV domains, see the only caller in pci_iommu_alloc(). So this is just
dead code.
> +
> if (!xen_initial_domain() && !x86_swiotlb_enable)
> return;
> x86_swiotlb_enable = true;
> @@ -86,7 +92,7 @@ static void __init pci_xen_swiotlb_init(void)
>
> int pci_xen_swiotlb_init_late(void)
> {
> - if (dma_ops == &xen_swiotlb_dma_ops)
> + if (xen_pvh_domain() || dma_ops == &xen_swiotlb_dma_ops)
Same here, this function is only called by
pcifront_connect_and_init_dma() and pcifront should never attach on a
PVH domain, hence it's also dead code.
Thanks, Roger.
Re: [RFC PATCH 5/5] xen/privcmd: add IOCTL_PRIVCMD_GSI_FROM_IRQ
On Sun, Mar 12, 2023 at 08:01:57PM +0800, Huang Rui wrote: > From: Chen Jiqian > > When hypervisor get an interrupt, it needs interrupt's > gsi number instead of irq number. Gsi number is unique > in xen, but irq number is only unique in one domain. > So, we need to record the relationship between irq and > gsi when dom0 initialized pci devices, and provide syscall > IOCTL_PRIVCMD_GSI_FROM_IRQ to translate irq to gsi. So > that, we can map pirq successfully in hypervisor side. GSI is not only unique in Xen, it's an ACPI provided value that's unique in the platform. The text above make it look like GSI is some kind of internal Xen reference to an interrupt, but it's not. How does a PV domain deal with this? I would assume there Linux will also end up with IRQ != GSI, and hence will need some kind of translation? Thanks, Roger.
Re: [RFC PATCH 4/5] x86/xen: acpi registers gsi for xen pvh
On Sun, Mar 12, 2023 at 08:01:56PM +0800, Huang Rui wrote: > From: Chen Jiqian > > Add acpi_register_gsi_xen_pvh() to register gsi for PVH mode. > In addition to call acpi_register_gsi_ioapic(), it also setup > a map between gsi and vector in hypervisor side. So that, > when dgpu create an interrupt, hypervisor can correctly find > which guest domain to process interrupt by vector. The term 'dgpu' needs clarifying or replacing by a more generic naming. Also, I would like to be able to get away from requiring dom0 to register the GSIs in this way. If you take a look at Xen, there's code in the emulated IO-APIC available to dom0 that already does this registering (see vioapic_hwdom_map_gsi() in Xen). I think the problem here is that the GSI used by the device you want to passthrough has never had it's pin unmasked in the IO-APIC, and hence hasn't been registered. It would be helpful if you could state in the commit message what issue you are trying to solve by doing this registering here, I assume it is done in order to map the IRQ to a PIRQ, so later calls by the toolstack to bind it succeed. Would it be possible instead to perform the call to PHYSDEVOP_map_pirq in the toolstack itself if the PIRQ cannot be found? Thanks, Roger.
Re: [RFC PATCH 3/5] drm/amdgpu: set passthrough mode for xen pvh/hvm
On Sun, Mar 12, 2023 at 08:01:55PM +0800, Huang Rui wrote: > There is an second stage translation between the guest machine address > and host machine address in Xen PVH/HVM. The PCI bar address in the xen > guest kernel are not translated at the second stage on Xen PVH/HVM, so I'm confused by the sentence above, do you think it could be reworded or expanded to clarify? PCI BAR addresses are not in the guest kernel, but rather in the physical memory layout made available to the guest. Also, I'm unsure why xen_initial_domain() needs to be used in the conditional below: all PV domains handle addresses the same, so if it's not needed for a PV dom0 it's likely not needed for a PV domU either. Albeit it would help to know more about what AMDGPU_PASSTHROUGH_MODE implies. Thanks, Roger.
Re: [RFC PATCH 2/5] xen/grants: update initialization order of xen grant table
On Sun, Mar 12, 2023 at 08:01:54PM +0800, Huang Rui wrote: > The xen grant table will be initialied before parsing the PCI resources, > so xen_alloc_unpopulated_pages() ends up using a range from the PCI > window because Linux hasn't parsed the PCI information yet. > > So modify the initialization order to make sure the real PCI resources > are parsed before. Has this been tested on a domU to make sure the late grant table init doesn't interfere with PV devices getting setup? > Signed-off-by: Huang Rui > --- > arch/x86/xen/grant-table.c | 2 +- > drivers/xen/grant-table.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c > index 1e681bf62561..64a04d1e70f5 100644 > --- a/arch/x86/xen/grant-table.c > +++ b/arch/x86/xen/grant-table.c > @@ -165,5 +165,5 @@ static int __init xen_pvh_gnttab_setup(void) > } > /* Call it _before_ __gnttab_init as we need to initialize the > * xen_auto_xlat_grant_frames first. */ > -core_initcall(xen_pvh_gnttab_setup); > +fs_initcall_sync(); > #endif > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c > index e1ec725c2819..6382112f3473 100644 > --- a/drivers/xen/grant-table.c > +++ b/drivers/xen/grant-table.c > @@ -1680,4 +1680,4 @@ static int __gnttab_init(void) > } > /* Starts after core_initcall so that xen_pvh_gnttab_setup can be called > * beforehand to initialize xen_auto_xlat_grant_frames. */ Comment need to be updated, but I was thinking whether it won't be best to simply call xen_pvh_gnttab_setup() from __gnttab_init() itself when running as a PVH guest? Thanks, Roger.
Re: [PATCH v5 3/3] xen: add helpers to allocate unpopulated memory
On Fri, Sep 04, 2020 at 09:00:18AM +0200, Jürgen Groß wrote:
> On 03.09.20 18:38, Roger Pau Monné wrote:
> > On Thu, Sep 03, 2020 at 05:30:07PM +0200, Jürgen Groß wrote:
> > > On 01.09.20 10:33, Roger Pau Monne wrote:
> > > > To be used in order to create foreign mappings. This is based on the
> > > > ZONE_DEVICE facility which is used by persistent memory devices in
> > > > order to create struct pages and kernel virtual mappings for the IOMEM
> > > > areas of such devices. Note that on kernels without support for
> > > > ZONE_DEVICE Xen will fallback to use ballooned pages in order to
> > > > create foreign mappings.
> > > >
> > > > The newly added helpers use the same parameters as the existing
> > > > {alloc/free}_xenballooned_pages functions, which allows for in-place
> > > > replacement of the callers. Once a memory region has been added to be
> > > > used as scratch mapping space it will no longer be released, and pages
> > > > returned are kept in a linked list. This allows to have a buffer of
> > > > pages and prevents resorting to frequent additions and removals of
> > > > regions.
> > > >
> > > > If enabled (because ZONE_DEVICE is supported) the usage of the new
> > > > functionality untangles Xen balloon and RAM hotplug from the usage of
> > > > unpopulated physical memory ranges to map foreign pages, which is the
> > > > correct thing to do in order to avoid mappings of foreign pages depend
> > > > on memory hotplug.
> > > >
> > > > Note the driver is currently not enabled on Arm platforms because it
> > > > would interfere with the identity mapping required on some platforms.
> > > >
> > > > Signed-off-by: Roger Pau Monné
> > >
> > > Sorry, I just got a build error for x86 32-bit build:
> > >
> > > WARNING: unmet direct dependencies detected for ZONE_DEVICE
> > >Depends on [n]: MEMORY_HOTPLUG [=n] && MEMORY_HOTREMOVE [=n] &&
> > > SPARSEMEM_VMEMMAP [=n] && ARCH_HAS_PTE_DEVMAP [=n]
> > >Selected by [y]:
> > >- XEN_UNPOPULATED_ALLOC [=y] && XEN [=y] && X86 [=y]
> > >GEN Makefile
> > >CC kernel/bounds.s
> > >CALL/home/gross/korg/src/scripts/atomic/check-atomics.sh
> > >UPD include/generated/bounds.h
> > >CC arch/x86/kernel/asm-offsets.s
> > > In file included from /home/gross/korg/src/include/linux/mmzone.h:19:0,
> > > from /home/gross/korg/src/include/linux/gfp.h:6,
> > > from /home/gross/korg/src/include/linux/slab.h:15,
> > > from /home/gross/korg/src/include/linux/crypto.h:19,
> > > from
> > > /home/gross/korg/src/arch/x86/kernel/asm-offsets.c:9:
> > > /home/gross/korg/src/include/linux/page-flags-layout.h:95:2: error: #error
> > > "Not enough bits in page flags"
> > > #error "Not enough bits in page flags"
> > >^
> > > make[2]: *** [/home/gross/korg/src/scripts/Makefile.build:114:
> > > arch/x86/kernel/asm-offsets.s] Error 1
> > > make[1]: *** [/home/gross/korg/src/Makefile:1175: prepare0] Error 2
> > > make[1]: Leaving directory '/home/gross/korg/x8632'
> > > make: *** [Makefile:185: __sub-make] Error 2
> >
> > Sorry for this. I've tested a 32bit build but I think it was before
> > the last Kconfig changes. I'm a little unsure how to solve this, as
> > ZONE_DEVICE doesn't select the required options for it to run, but
> > rather depends on them to be available.
> >
> > You can trigger something similar on x86-64 by doing:
> >
> > $ make ARCH=x86_64 xen.config
> > Using .config as base
> > Merging ./kernel/configs/xen.config
> > Merging ./arch/x86/configs/xen.config
> > #
> > # merged configuration written to .config (needs make)
> > #
> > scripts/kconfig/conf --olddefconfig Kconfig
> >
> > WARNING: unmet direct dependencies detected for ZONE_DEVICE
> >Depends on [n]: MEMORY_HOTPLUG [=y] && MEMORY_HOTREMOVE [=n] &&
> > SPARSEMEM_VMEMMAP [=y] && ARCH_HAS_PTE_DEVMAP [=y]
> >Selected by [y]:
> >- XEN_UNPOPULATED_ALLOC [=y] && XEN [=y] && X86_64 [=y]
> > #
> > # configuration written to .config
> > #
> >
> > I think the only solution is to have XEN_UNPOPULATED_ALLOC depend on
> > ZONE_DEVICE rather than select it?
>
> Yes, I think so.
>
> I've folded that in and now build is fine.
Thanks, I assume no further action is needed on my side.
Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 3/3] xen: add helpers to allocate unpopulated memory
On Thu, Sep 03, 2020 at 05:30:07PM +0200, Jürgen Groß wrote:
> On 01.09.20 10:33, Roger Pau Monne wrote:
> > To be used in order to create foreign mappings. This is based on the
> > ZONE_DEVICE facility which is used by persistent memory devices in
> > order to create struct pages and kernel virtual mappings for the IOMEM
> > areas of such devices. Note that on kernels without support for
> > ZONE_DEVICE Xen will fallback to use ballooned pages in order to
> > create foreign mappings.
> >
> > The newly added helpers use the same parameters as the existing
> > {alloc/free}_xenballooned_pages functions, which allows for in-place
> > replacement of the callers. Once a memory region has been added to be
> > used as scratch mapping space it will no longer be released, and pages
> > returned are kept in a linked list. This allows to have a buffer of
> > pages and prevents resorting to frequent additions and removals of
> > regions.
> >
> > If enabled (because ZONE_DEVICE is supported) the usage of the new
> > functionality untangles Xen balloon and RAM hotplug from the usage of
> > unpopulated physical memory ranges to map foreign pages, which is the
> > correct thing to do in order to avoid mappings of foreign pages depend
> > on memory hotplug.
> >
> > Note the driver is currently not enabled on Arm platforms because it
> > would interfere with the identity mapping required on some platforms.
> >
> > Signed-off-by: Roger Pau Monné
>
> Sorry, I just got a build error for x86 32-bit build:
>
> WARNING: unmet direct dependencies detected for ZONE_DEVICE
> Depends on [n]: MEMORY_HOTPLUG [=n] && MEMORY_HOTREMOVE [=n] &&
> SPARSEMEM_VMEMMAP [=n] && ARCH_HAS_PTE_DEVMAP [=n]
> Selected by [y]:
> - XEN_UNPOPULATED_ALLOC [=y] && XEN [=y] && X86 [=y]
> GEN Makefile
> CC kernel/bounds.s
> CALL/home/gross/korg/src/scripts/atomic/check-atomics.sh
> UPD include/generated/bounds.h
> CC arch/x86/kernel/asm-offsets.s
> In file included from /home/gross/korg/src/include/linux/mmzone.h:19:0,
> from /home/gross/korg/src/include/linux/gfp.h:6,
> from /home/gross/korg/src/include/linux/slab.h:15,
> from /home/gross/korg/src/include/linux/crypto.h:19,
> from /home/gross/korg/src/arch/x86/kernel/asm-offsets.c:9:
> /home/gross/korg/src/include/linux/page-flags-layout.h:95:2: error: #error
> "Not enough bits in page flags"
> #error "Not enough bits in page flags"
> ^
> make[2]: *** [/home/gross/korg/src/scripts/Makefile.build:114:
> arch/x86/kernel/asm-offsets.s] Error 1
> make[1]: *** [/home/gross/korg/src/Makefile:1175: prepare0] Error 2
> make[1]: Leaving directory '/home/gross/korg/x8632'
> make: *** [Makefile:185: __sub-make] Error 2
Sorry for this. I've tested a 32bit build but I think it was before
the last Kconfig changes. I'm a little unsure how to solve this, as
ZONE_DEVICE doesn't select the required options for it to run, but
rather depends on them to be available.
You can trigger something similar on x86-64 by doing:
$ make ARCH=x86_64 xen.config
Using .config as base
Merging ./kernel/configs/xen.config
Merging ./arch/x86/configs/xen.config
#
# merged configuration written to .config (needs make)
#
scripts/kconfig/conf --olddefconfig Kconfig
WARNING: unmet direct dependencies detected for ZONE_DEVICE
Depends on [n]: MEMORY_HOTPLUG [=y] && MEMORY_HOTREMOVE [=n] &&
SPARSEMEM_VMEMMAP [=y] && ARCH_HAS_PTE_DEVMAP [=y]
Selected by [y]:
- XEN_UNPOPULATED_ALLOC [=y] && XEN [=y] && X86_64 [=y]
#
# configuration written to .config
#
I think the only solution is to have XEN_UNPOPULATED_ALLOC depend on
ZONE_DEVICE rather than select it?
Thanks, Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 3/3] xen: add helpers to allocate unpopulated memory
On Tue, Sep 01, 2020 at 10:33:26AM +0200, Roger Pau Monne wrote:
> +static int fill_list(unsigned int nr_pages)
> +{
> + struct dev_pagemap *pgmap;
> + void *vaddr;
> + unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
> + int nid, ret;
> +
> + pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
> + if (!pgmap)
> + return -ENOMEM;
> +
> + pgmap->type = MEMORY_DEVICE_GENERIC;
> + pgmap->res.name = "Xen scratch";
> + pgmap->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> +
> + ret = allocate_resource(&iomem_resource, &pgmap->res,
> + alloc_pages * PAGE_SIZE, 0, -1,
> + PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
> + if (ret < 0) {
> + pr_err("Cannot allocate new IOMEM resource\n");
> + kfree(pgmap);
> + return ret;
> + }
> +
> + nid = memory_add_physaddr_to_nid(pgmap->res.start);
I think this is not needed ...
> +
> +#ifdef CONFIG_XEN_HAVE_PVMMU
> +/*
> + * memremap will build page tables for the new memory so
> + * the p2m must contain invalid entries so the correct
> + * non-present PTEs will be written.
> + *
> + * If a failure occurs, the original (identity) p2m entries
> + * are not restored since this region is now known not to
> + * conflict with any devices.
> + */
> + if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> + xen_pfn_t pfn = PFN_DOWN(pgmap->res.start);
> +
> + for (i = 0; i < alloc_pages; i++) {
> + if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
> + pr_warn("set_phys_to_machine() failed, no
> memory added\n");
> + release_resource(&pgmap->res);
> + kfree(pgmap);
> + return -ENOMEM;
> + }
> +}
> + }
> +#endif
> +
> + vaddr = memremap_pages(pgmap, nid);
... and NUMA_NO_NODE should be used here instead, as this memory is just
fictitious space to map foreign memory, and shouldn't be related to
any NUMA node.
The following chunk should be folded in, or I can resend.
Thanks, Roger.
---8<---
diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c
index 1b5d157c6977..3b98dc921426 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -20,7 +20,7 @@ static int fill_list(unsigned int nr_pages)
struct dev_pagemap *pgmap;
void *vaddr;
unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
- int nid, ret;
+ int ret;
pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
if (!pgmap)
@@ -39,8 +39,6 @@ static int fill_list(unsigned int nr_pages)
return ret;
}
- nid = memory_add_physaddr_to_nid(pgmap->res.start);
-
#ifdef CONFIG_XEN_HAVE_PVMMU
/*
* memremap will build page tables for the new memory so
@@ -65,7 +63,7 @@ static int fill_list(unsigned int nr_pages)
}
#endif
- vaddr = memremap_pages(pgmap, nid);
+ vaddr = memremap_pages(pgmap, NUMA_NO_NODE);
if (IS_ERR(vaddr)) {
pr_err("Cannot remap memory range\n");
release_resource(&pgmap->res);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory
On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote: > On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote: > > If enabled (because ZONE_DEVICE is supported) the usage of the new > > functionality untangles Xen balloon and RAM hotplug from the usage of > > unpopulated physical memory ranges to map foreign pages, which is the > > correct thing to do in order to avoid mappings of foreign pages depend > > on memory hotplug. > > So please just select ZONE_DEVICE if this is so much better rather > than maintaining two variants. We still need to other variant for Arm at least, so both need to be maintained anyway, even if we force ZONE_DEVICE on x86. Thanks, Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory
On Fri, Aug 14, 2020 at 02:54:38PM +0200, Jürgen Groß wrote: > On 14.08.20 14:47, Roger Pau Monné wrote: > > On Fri, Aug 14, 2020 at 12:27:32PM +0200, Jürgen Groß wrote: > > > On 14.08.20 11:56, Roger Pau Monné wrote: > > > > On Fri, Aug 14, 2020 at 08:29:20AM +0100, Christoph Hellwig wrote: > > > > > On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monn?? wrote: > > > > > > On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote: > > > > > > > On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote: > > > > > > > > If enabled (because ZONE_DEVICE is supported) the usage of the > > > > > > > > new > > > > > > > > functionality untangles Xen balloon and RAM hotplug from the > > > > > > > > usage of > > > > > > > > unpopulated physical memory ranges to map foreign pages, which > > > > > > > > is the > > > > > > > > correct thing to do in order to avoid mappings of foreign pages > > > > > > > > depend > > > > > > > > on memory hotplug. > > > > > > > > > > > > > > So please just select ZONE_DEVICE if this is so much better rather > > > > > > > than maintaining two variants. > > > > > > > > > > > > We still need to other variant for Arm at least, so both need to be > > > > > > maintained anyway, even if we force ZONE_DEVICE on x86. > > > > > > > > > > Well, it still really helps reproducability if you stick to one > > > > > implementation of x86. > > > > > > > > > > The alternative would be an explicit config option to opt into it, > > > > > but just getting a different implementation based on a random > > > > > kernel option is strange. > > > > > > > > Would adding something like the chunk below to the patch be OK? > > > > > > > > ---8<--- > > > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig > > > > index 018020b91baa..5f321a1319e6 100644 > > > > --- a/drivers/xen/Kconfig > > > > +++ b/drivers/xen/Kconfig > > > > @@ -328,7 +328,14 @@ config XEN_FRONT_PGDIR_SHBUF > > > > tristate > > > >config XEN_UNPOPULATED_ALLOC > > > > - bool > > > > - default y if ZONE_DEVICE && !ARM && !ARM64 > > > > + bool "Use unpopulated memory ranges for guest mappings" > > > > + depends on X86 > > > > + select ZONE_DEVICE > > > > + default y > > > > > > I'd rather use "default XEN_BACKEND" here, as mappings of other guest's > > > memory is rarely used for non-backend guests. > > > > There's also the privcmd and gnt devices which make heavy use of this, > > so I'm not sure only selecting by default on XEN_BACKEND is the best > > option. > > I just want to avoid that kernels built for running as Xen guest, but > not as dom0, will be forced to select ZONE_DEVICE. > > As privcmd is dom0-only, this is no problem. Oh, didn't know that, I somehow assumed privcmd would be available to all Xen guests regardless of whether dom0 support was selected. > In case you are worrying about gnt devices, I'd be fine to switch to > > default XEN_BACKEND || XEN_GNTDEV Sure. maybe even: default XEN_BACKEND || XEN_GNTDEV || XEN_DOM0 Do you want me to resend with this changed or are you happy to fixup if there are no further comments? Thanks, Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory
On Fri, Aug 14, 2020 at 12:27:32PM +0200, Jürgen Groß wrote: > On 14.08.20 11:56, Roger Pau Monné wrote: > > On Fri, Aug 14, 2020 at 08:29:20AM +0100, Christoph Hellwig wrote: > > > On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monn?? wrote: > > > > On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote: > > > > > On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote: > > > > > > If enabled (because ZONE_DEVICE is supported) the usage of the new > > > > > > functionality untangles Xen balloon and RAM hotplug from the usage > > > > > > of > > > > > > unpopulated physical memory ranges to map foreign pages, which is > > > > > > the > > > > > > correct thing to do in order to avoid mappings of foreign pages > > > > > > depend > > > > > > on memory hotplug. > > > > > > > > > > So please just select ZONE_DEVICE if this is so much better rather > > > > > than maintaining two variants. > > > > > > > > We still need to other variant for Arm at least, so both need to be > > > > maintained anyway, even if we force ZONE_DEVICE on x86. > > > > > > Well, it still really helps reproducability if you stick to one > > > implementation of x86. > > > > > > The alternative would be an explicit config option to opt into it, > > > but just getting a different implementation based on a random > > > kernel option is strange. > > > > Would adding something like the chunk below to the patch be OK? > > > > ---8<--- > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig > > index 018020b91baa..5f321a1319e6 100644 > > --- a/drivers/xen/Kconfig > > +++ b/drivers/xen/Kconfig > > @@ -328,7 +328,14 @@ config XEN_FRONT_PGDIR_SHBUF > > tristate > > config XEN_UNPOPULATED_ALLOC > > - bool > > - default y if ZONE_DEVICE && !ARM && !ARM64 > > + bool "Use unpopulated memory ranges for guest mappings" > > + depends on X86 > > + select ZONE_DEVICE > > + default y > > I'd rather use "default XEN_BACKEND" here, as mappings of other guest's > memory is rarely used for non-backend guests. There's also the privcmd and gnt devices which make heavy use of this, so I'm not sure only selecting by default on XEN_BACKEND is the best option. Thanks, Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory
On Fri, Aug 14, 2020 at 08:29:20AM +0100, Christoph Hellwig wrote: > On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monn?? wrote: > > On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote: > > > On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote: > > > > If enabled (because ZONE_DEVICE is supported) the usage of the new > > > > functionality untangles Xen balloon and RAM hotplug from the usage of > > > > unpopulated physical memory ranges to map foreign pages, which is the > > > > correct thing to do in order to avoid mappings of foreign pages depend > > > > on memory hotplug. > > > > > > So please just select ZONE_DEVICE if this is so much better rather > > > than maintaining two variants. > > > > We still need to other variant for Arm at least, so both need to be > > maintained anyway, even if we force ZONE_DEVICE on x86. > > Well, it still really helps reproducability if you stick to one > implementation of x86. > > The alternative would be an explicit config option to opt into it, > but just getting a different implementation based on a random > kernel option is strange. Would adding something like the chunk below to the patch be OK? ---8<--- diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig index 018020b91baa..5f321a1319e6 100644 --- a/drivers/xen/Kconfig +++ b/drivers/xen/Kconfig @@ -328,7 +328,14 @@ config XEN_FRONT_PGDIR_SHBUF tristate config XEN_UNPOPULATED_ALLOC - bool - default y if ZONE_DEVICE && !ARM && !ARM64 + bool "Use unpopulated memory ranges for guest mappings" + depends on X86 + select ZONE_DEVICE + default y + help + Use unpopulated memory ranges in order to create mappings for guest + memory regions, including grants maps and foreign pages. This avoids + having to balloon out RAM regions in order to obtain physical memory + space to create such mappings. endmenu ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory
Your email client seems to set 'Reply-to:' to point to everyone on the 'Cc:' field, but not yourself, which is kind of weird. I've manually fixed it on this reply by moving everyone to the 'Cc:' field and setting you on 'To:'. On Thu, Aug 13, 2020 at 11:49:46AM +0200, Daniel Vetter wrote: > On Thu, Aug 13, 2020 at 09:54:20AM +0200, Roger Pau Monné wrote: > > On Thu, Aug 13, 2020 at 08:33:37AM +0100, Christoph Hellwig wrote: > > > On Tue, Aug 11, 2020 at 11:44:47AM +0200, Roger Pau Monne wrote: > > > > If enabled (because ZONE_DEVICE is supported) the usage of the new > > > > functionality untangles Xen balloon and RAM hotplug from the usage of > > > > unpopulated physical memory ranges to map foreign pages, which is the > > > > correct thing to do in order to avoid mappings of foreign pages depend > > > > on memory hotplug. > > > > > > So please just select ZONE_DEVICE if this is so much better rather > > > than maintaining two variants. > > > > We still need to other variant for Arm at least, so both need to be > > maintained anyway, even if we force ZONE_DEVICE on x86. > > Why does arm not have ZONE_DEVICE? It's not that Arm doesn't have ZONE_DEVICE, it's just that the approach used here won't work correctly on an Arm Xen dom0 as-is. This is due to the usage of an identity second stage translation in order to workaround the lack of an IOMMU in some Arm boards. It can be made to work on Arm, but will likely require someone from the Arm side doing that. Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/2] xen: add helpers to allocate unpopulated memory
On Wed, Aug 12, 2020 at 09:28:45AM +0200, Jürgen Groß wrote:
> On 11.08.20 11:44, Roger Pau Monne wrote:
> > To be used in order to create foreign mappings. This is based on the
> > ZONE_DEVICE facility which is used by persistent memory devices in
> > order to create struct pages and kernel virtual mappings for the IOMEM
> > areas of such devices. Note that on kernels without support for
> > ZONE_DEVICE Xen will fallback to use ballooned pages in order to
> > create foreign mappings.
> >
> > The newly added helpers use the same parameters as the existing
> > {alloc/free}_xenballooned_pages functions, which allows for in-place
> > replacement of the callers. Once a memory region has been added to be
> > used as scratch mapping space it will no longer be released, and pages
> > returned are kept in a linked list. This allows to have a buffer of
> > pages and prevents resorting to frequent additions and removals of
> > regions.
> >
> > If enabled (because ZONE_DEVICE is supported) the usage of the new
> > functionality untangles Xen balloon and RAM hotplug from the usage of
> > unpopulated physical memory ranges to map foreign pages, which is the
> > correct thing to do in order to avoid mappings of foreign pages depend
> > on memory hotplug.
> >
> > Note the driver is currently not enabled on Arm platforms because it
> > would interfere with the identity mapping required on some platforms.
> >
> > Signed-off-by: Roger Pau Monné
> > ---
> > Cc: Oleksandr Andrushchenko
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Boris Ostrovsky
> > Cc: Juergen Gross
> > Cc: Stefano Stabellini
> > Cc: Dan Carpenter
> > Cc: Roger Pau Monne
> > Cc: Wei Liu
> > Cc: Yan Yankovskyi
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: xen-de...@lists.xenproject.org
> > Cc: linux...@kvack.org
> > Cc: David Hildenbrand
> > Cc: Michal Hocko
> > Cc: Dan Williams
> > ---
> > Changes since v3:
> > - Introduce a Kconfig option that gates the addition of the
> > unpopulated alloc driver. This allows to easily disable it on Arm
> > platforms.
> > - Dropped Juergen RB due to the addition of the Kconfig option.
> > - Switched from MEMORY_DEVICE_DEVDAX to MEMORY_DEVICE_GENERIC.
> >
> > Changes since v2:
> > - Drop BUILD_BUG_ON regarding PVMMU page sizes.
> > - Use a SPDX license identifier.
> > - Call fill with only the minimum required number of pages.
> > - Include xen.h header in xen_drm_front_gem.c.
> > - Use less generic function names.
> > - Exit early from the init function if not a PV guest.
> > - Don't use all caps for region name.
> > ---
> > drivers/gpu/drm/xen/xen_drm_front_gem.c | 9 +-
> > drivers/xen/Kconfig | 4 +
> > drivers/xen/Makefile| 1 +
> > drivers/xen/balloon.c | 4 +-
> > drivers/xen/grant-table.c | 4 +-
> > drivers/xen/privcmd.c | 4 +-
> > drivers/xen/unpopulated-alloc.c | 185
> > drivers/xen/xenbus/xenbus_client.c | 6 +-
> > drivers/xen/xlate_mmu.c | 4 +-
> > include/xen/xen.h | 9 ++
> > 10 files changed, 215 insertions(+), 15 deletions(-)
> > create mode 100644 drivers/xen/unpopulated-alloc.c
> >
> > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > index 1d339ef92422..018020b91baa 100644
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -327,4 +327,8 @@ config XEN_HAVE_VPMU
> > config XEN_FRONT_PGDIR_SHBUF
> > tristate
> > +config XEN_UNPOPULATED_ALLOC
> > + bool
> > + default y if ZONE_DEVICE && !ARM && !ARM64
>
> There is a current effort to enable Xen on RISC-V. Do we expect this
> option to be usable for this architecture?
It will depend on whether the Xen RISC-V implementation mandates an
IOMMU, for example Arm doesn't and hence uses an identity p2m for
dom0. If RISC-V does the same then I would assume this won't be
suitable as-is (not that it couldn't be made to work).
IMO it wasn't clear from the community call what was the RISC-V port
position regarding this, but IIRC the IOMMU spec for RISC-V was behind
the virtualization one, so it's likely that quite a lot of hardware
will have hardware virtualization support but no IOMMU, in which case
I think it's likely the RISC-V port will follow Arm and implement an
identity p2m.
> If yes, I'm fine with the
> exclusion of Arm, otherwise I'd opt for defaulting to yes only for
> X86.
>
> Either way you can have my:
>
> Reviewed-by: Juergen Gross
Thanks. Feel free to change the 'ZONE_DEVICE && !ARM && !ARM64' to
'ZONE_DEVICE && X86' if you think it's safer.
Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] xen: add helpers to allocate unpopulated memory
On Tue, Jul 28, 2020 at 06:12:46PM +0100, Julien Grall wrote:
> Hi Roger,
>
> On 28/07/2020 17:59, Roger Pau Monné wrote:
> > On Tue, Jul 28, 2020 at 05:48:23PM +0100, Julien Grall wrote:
> > > Hi,
> > >
> > > On 27/07/2020 10:13, Roger Pau Monne wrote:
> > > > To be used in order to create foreign mappings. This is based on the
> > > > ZONE_DEVICE facility which is used by persistent memory devices in
> > > > order to create struct pages and kernel virtual mappings for the IOMEM
> > > > areas of such devices. Note that on kernels without support for
> > > > ZONE_DEVICE Xen will fallback to use ballooned pages in order to
> > > > create foreign mappings.
> > > >
> > > > The newly added helpers use the same parameters as the existing
> > > > {alloc/free}_xenballooned_pages functions, which allows for in-place
> > > > replacement of the callers. Once a memory region has been added to be
> > > > used as scratch mapping space it will no longer be released, and pages
> > > > returned are kept in a linked list. This allows to have a buffer of
> > > > pages and prevents resorting to frequent additions and removals of
> > > > regions.
> > > >
> > > > If enabled (because ZONE_DEVICE is supported) the usage of the new
> > > > functionality untangles Xen balloon and RAM hotplug from the usage of
> > > > unpopulated physical memory ranges to map foreign pages, which is the
> > > > correct thing to do in order to avoid mappings of foreign pages depend
> > > > on memory hotplug.
> > > I think this is going to break Dom0 on Arm if the kernel has been built
> > > with
> > > hotplug. This is because you may end up to re-use region that will be used
> > > for the 1:1 mapping of a foreign map.
> > >
> > > Note that I don't know whether hotplug has been tested on Xen on Arm yet.
> > > So
> > > it might be possible to be already broken.
> > >
> > > Meanwhile, my suggestion would be to make the use of hotplug in the
> > > balloon
> > > code conditional (maybe using CONFIG_ARM64 and CONFIG_ARM)?
> >
> > Right, this feature (allocation of unpopulated memory separated from
> > the balloon driver) is currently gated on CONFIG_ZONE_DEVICE, which I
> > think could be used on Arm.
> >
> > IMO the right solution seems to be to subtract the physical memory
> > regions that can be used for the identity mappings of foreign pages
> > (all RAM on the system AFAICT) from iomem_resource, as that would make
> > this and the memory hotplug done in the balloon driver safe?
>
> Dom0 doesn't know the regions used for the identity mappings as this is only
> managed by Xen. So there is nothing you can really do here.
OK, I will add the guards to prevent this being built on Arm.
> But don't you have the same issue on x86 with "magic pages"?
Those are marked as reserved on the memory map, and hence I would
expect them to never end up in iomem_resource.
Thanks, Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] xen: add helpers to allocate unpopulated memory
On Tue, Jul 28, 2020 at 06:06:25PM +0100, Andrew Cooper wrote:
> On 28/07/2020 17:59, Roger Pau Monné wrote:
> > On Tue, Jul 28, 2020 at 05:48:23PM +0100, Julien Grall wrote:
> >> Hi,
> >>
> >> On 27/07/2020 10:13, Roger Pau Monne wrote:
> >>> To be used in order to create foreign mappings. This is based on the
> >>> ZONE_DEVICE facility which is used by persistent memory devices in
> >>> order to create struct pages and kernel virtual mappings for the IOMEM
> >>> areas of such devices. Note that on kernels without support for
> >>> ZONE_DEVICE Xen will fallback to use ballooned pages in order to
> >>> create foreign mappings.
> >>>
> >>> The newly added helpers use the same parameters as the existing
> >>> {alloc/free}_xenballooned_pages functions, which allows for in-place
> >>> replacement of the callers. Once a memory region has been added to be
> >>> used as scratch mapping space it will no longer be released, and pages
> >>> returned are kept in a linked list. This allows to have a buffer of
> >>> pages and prevents resorting to frequent additions and removals of
> >>> regions.
> >>>
> >>> If enabled (because ZONE_DEVICE is supported) the usage of the new
> >>> functionality untangles Xen balloon and RAM hotplug from the usage of
> >>> unpopulated physical memory ranges to map foreign pages, which is the
> >>> correct thing to do in order to avoid mappings of foreign pages depend
> >>> on memory hotplug.
> >> I think this is going to break Dom0 on Arm if the kernel has been built
> >> with
> >> hotplug. This is because you may end up to re-use region that will be used
> >> for the 1:1 mapping of a foreign map.
> >>
> >> Note that I don't know whether hotplug has been tested on Xen on Arm yet.
> >> So
> >> it might be possible to be already broken.
> >>
> >> Meanwhile, my suggestion would be to make the use of hotplug in the balloon
> >> code conditional (maybe using CONFIG_ARM64 and CONFIG_ARM)?
> > Right, this feature (allocation of unpopulated memory separated from
> > the balloon driver) is currently gated on CONFIG_ZONE_DEVICE, which I
> > think could be used on Arm.
> >
> > IMO the right solution seems to be to subtract the physical memory
> > regions that can be used for the identity mappings of foreign pages
> > (all RAM on the system AFAICT) from iomem_resource, as that would make
> > this and the memory hotplug done in the balloon driver safe?
>
> The right solution is a mechanism for translated guests to query Xen to
> find regions of guest physical address space which are unused, and can
> be safely be used for foreign/grant/other mappings.
>
> Please don't waste any more time applying more duct tape to a broken
> system, and instead spend the time organising some proper foundations.
The piece added here (using ZONE_DEVICE) will be relevant when Xen can
provide the space to map foreign pages, it's just that right now it
relies on iomem_resource instead of a Xen specific resource map that
should be provided by the hypervisor. It should indeed be fixed, but
right now this patch should allow a PVH dom0 to work slightly better.
When Xen provides such areas Linux just needs to populate a custom Xen
resource with them and use it instead of iomem_resurce.
The Arm stuff I'm certainly not familiar with, and can't provide much
insight on that. If it's best to just disable it and continue to rely
on ballooned out pages that's fine.
Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] xen: add helpers to allocate unpopulated memory
On Tue, Jul 28, 2020 at 05:48:23PM +0100, Julien Grall wrote:
> Hi,
>
> On 27/07/2020 10:13, Roger Pau Monne wrote:
> > To be used in order to create foreign mappings. This is based on the
> > ZONE_DEVICE facility which is used by persistent memory devices in
> > order to create struct pages and kernel virtual mappings for the IOMEM
> > areas of such devices. Note that on kernels without support for
> > ZONE_DEVICE Xen will fallback to use ballooned pages in order to
> > create foreign mappings.
> >
> > The newly added helpers use the same parameters as the existing
> > {alloc/free}_xenballooned_pages functions, which allows for in-place
> > replacement of the callers. Once a memory region has been added to be
> > used as scratch mapping space it will no longer be released, and pages
> > returned are kept in a linked list. This allows to have a buffer of
> > pages and prevents resorting to frequent additions and removals of
> > regions.
> >
> > If enabled (because ZONE_DEVICE is supported) the usage of the new
> > functionality untangles Xen balloon and RAM hotplug from the usage of
> > unpopulated physical memory ranges to map foreign pages, which is the
> > correct thing to do in order to avoid mappings of foreign pages depend
> > on memory hotplug.
> I think this is going to break Dom0 on Arm if the kernel has been built with
> hotplug. This is because you may end up to re-use region that will be used
> for the 1:1 mapping of a foreign map.
>
> Note that I don't know whether hotplug has been tested on Xen on Arm yet. So
> it might be possible to be already broken.
>
> Meanwhile, my suggestion would be to make the use of hotplug in the balloon
> code conditional (maybe using CONFIG_ARM64 and CONFIG_ARM)?
Right, this feature (allocation of unpopulated memory separated from
the balloon driver) is currently gated on CONFIG_ZONE_DEVICE, which I
think could be used on Arm.
IMO the right solution seems to be to subtract the physical memory
regions that can be used for the identity mappings of foreign pages
(all RAM on the system AFAICT) from iomem_resource, as that would make
this and the memory hotplug done in the balloon driver safe?
Thanks, Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/4] xen: add helpers to allocate unpopulated memory
On Fri, Jul 24, 2020 at 12:36:33PM -0400, Boris Ostrovsky wrote:
> On 7/24/20 10:34 AM, David Hildenbrand wrote:
> > CCing Dan
> >
> > On 24.07.20 14:42, Roger Pau Monne wrote:
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include
> >> +
> >> +#include
> >> +#include
> >> +
> >> +static DEFINE_MUTEX(lock);
> >> +static LIST_HEAD(list);
> >> +static unsigned int count;
> >> +
> >> +static int fill(unsigned int nr_pages)
>
>
> Less generic names? How about list_lock, pg_list, pg_count,
> fill_pglist()? (But these are bad too, so maybe you can come up with
> something better)
OK, I have to admit I like using such short names when the code allows
to, for example this code is so simple that it didn't seem to warrant
using longer names. Will rename on next version.
> >> +{
> >> + struct dev_pagemap *pgmap;
> >> + void *vaddr;
> >> + unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
> >> + int nid, ret;
> >> +
> >> + pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
> >> + if (!pgmap)
> >> + return -ENOMEM;
> >> +
> >> + pgmap->type = MEMORY_DEVICE_DEVDAX;
> >> + pgmap->res.name = "XEN SCRATCH";
>
>
> Typically iomem resources only capitalize first letters.
>
>
> >> + pgmap->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> >> +
> >> + ret = allocate_resource(&iomem_resource, &pgmap->res,
> >> + alloc_pages * PAGE_SIZE, 0, -1,
> >> + PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
>
>
> Are we not going to end up with a whole bunch of "Xen scratch" resource
> ranges for each miss in the page list? Or do we expect them to get merged?
PAGES_PER_SECTION is IMO big enough to prevent ending up with a lot of
separated ranges. I think the value is 32 or 64MiB on x86, so while we
are likely to end up with more than one resource added, I don't think
it's going to be massive.
>
> >> + if (ret < 0) {
> >> + pr_err("Cannot allocate new IOMEM resource\n");
> >> + kfree(pgmap);
> >> + return ret;
> >> + }
> >> +
> >> + nid = memory_add_physaddr_to_nid(pgmap->res.start);
>
>
> Should we consider page range crossing node boundaries?
I'm not sure whether this is possible (I would think allocate_resource
should return a range from a single node), but then it would greatly
complicate the code to perform the memremap_pages, as we would have to
split the region into multiple dev_pagemap structs.
FWIW the current code in the balloon driver does exactly the same
(which doesn't mean it's correct, but that's where I got the logic
from).
> >> +
> >> +#ifdef CONFIG_XEN_HAVE_PVMMU
> >> + /*
> >> + * We don't support PV MMU when Linux and Xen is using
> >> + * different page granularity.
> >> + */
> >> + BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
> >> +
> >> +/*
> >> + * memremap will build page tables for the new memory so
> >> + * the p2m must contain invalid entries so the correct
> >> + * non-present PTEs will be written.
> >> + *
> >> + * If a failure occurs, the original (identity) p2m entries
> >> + * are not restored since this region is now known not to
> >> + * conflict with any devices.
> >> + */
> >> + if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> >> + xen_pfn_t pfn = PFN_DOWN(pgmap->res.start);
> >> +
> >> + for (i = 0; i < alloc_pages; i++) {
> >> + if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
> >> + pr_warn("set_phys_to_machine() failed, no
> >> memory added\n");
> >> + release_resource(&pgmap->res);
> >> + kfree(pgmap);
> >> + return -ENOMEM;
> >> + }
> >> +}
> >> + }
> >> +#endif
> >> +
> >> + vaddr = memremap_pages(pgmap, nid);
> >> + if (IS_ERR(vaddr)) {
> >> + pr_err("Cannot remap memory range\n");
> >> + release_resource(&pgmap->res);
> >> + kfree(pgmap);
> >> + return PTR_ERR(vaddr);
> >> + }
> >> +
> >> + for (i = 0; i < alloc_pages; i++) {
> >> + struct page *pg = virt_to_page(vaddr + PAGE_SIZE * i);
> >> +
> >> + BUG_ON(!virt_addr_valid(vaddr + PAGE_SIZE * i));
> >> + list_add(&pg->lru, &list);
> >> + count++;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * xen_alloc_unpopulated_pages - alloc unpopulated pages
> >> + * @nr_pages: Number of pages
> >> + * @pages: pages returned
> >> + * @return 0 on success, error otherwise
> >> + */
> >> +int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page
> >> **pages)
> >> +{
> >> + unsigned int i;
> >> + int ret = 0;
> >> +
> >> + mutex_lock(&lock);
> >> + if (count < nr_pages) {
> >> + ret = fill(nr_pages);
>
>
> (nr_pages - count) ?
Yup, already fixed as Juergen also pointed it out.
> >> +
Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver
On Wed, Apr 18, 2018 at 09:38:39AM +0300, Oleksandr Andrushchenko wrote: > On 04/17/2018 11:57 PM, Dongwon Kim wrote: > > On Tue, Apr 17, 2018 at 09:59:28AM +0200, Daniel Vetter wrote: > > > On Mon, Apr 16, 2018 at 12:29:05PM -0700, Dongwon Kim wrote: > 3.2 Backend exports dma-buf to xen-front > > In this case Dom0 pages are shared with DomU. As before, DomU can only write > to these pages, not any other page from Dom0, so it can be still considered > safe. > But, the following must be considered (highlighted in xen-front's Kernel > documentation): > - If guest domain dies then pages/grants received from the backend cannot > be claimed back - think of it as memory lost to Dom0 (won't be used for > any > other guest) > - Misbehaving guest may send too many requests to the backend exhausting > its grant references and memory (consider this from security POV). As the > backend runs in the trusted domain we also assume that it is trusted as > well, > e.g. must take measures to prevent DDoS attacks. I cannot parse the above sentence: "As the backend runs in the trusted domain we also assume that it is trusted as well, e.g. must take measures to prevent DDoS attacks." What's the relation between being trusted and protecting from DoS attacks? In any case, all? PV protocols are implemented with the frontend sharing pages to the backend, and I think there's a reason why this model is used, and it should continue to be used. Having to add logic in the backend to prevent such attacks means that: - We need more code in the backend, which increases complexity and chances of bugs. - Such code/logic could be wrong, thus allowing DoS. > 4. xen-front/backend/xen-zcopy synchronization > > 4.1. As I already said in 2) all the inter VM communication happens between > xen-front and the backend, xen-zcopy is NOT involved in that. > When xen-front wants to destroy a display buffer (dumb/dma-buf) it issues a > XENDISPL_OP_DBUF_DESTROY command (opposite to XENDISPL_OP_DBUF_CREATE). > This call is synchronous, so xen-front expects that backend does free the > buffer pages on return. > > 4.2. Backend, on XENDISPL_OP_DBUF_DESTROY: > - closes all dumb handles/fd's of the buffer according to [3] > - issues DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE IOCTL to xen-zcopy to make > sure > the buffer is freed (think of it as it waits for dma-buf->release > callback) So this zcopy thing keeps some kind of track of the memory usage? Why can't the user-space backend keep track of the buffer usage? > - replies to xen-front that the buffer can be destroyed. > This way deletion of the buffer happens synchronously on both Dom0 and DomU > sides. In case if DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE returns with time-out > error > (BTW, wait time is a parameter of this IOCTL), Xen will defer grant > reference > removal and will retry later until those are free. > > Hope this helps understand how buffers are synchronously deleted in case > of xen-zcopy with a single protocol command. > > I think the above logic can also be re-used by the hyper-dmabuf driver with > some additional work: > > 1. xen-zcopy can be split into 2 parts and extend: > 1.1. Xen gntdev driver [4], [5] to allow creating dma-buf from grefs and > vise versa, I don't know much about the dma-buf implementation in Linux, but gntdev is a user-space device, and AFAICT user-space applications don't have any notion of dma buffers. How are such buffers useful for user-space? Why can't this just be called memory? Also, (with my FreeBSD maintainer hat) how is this going to translate to other OSes? So far the operations performed by the gntdev device are mostly OS-agnostic because this just map/unmap memory, and in fact they are implemented by Linux and FreeBSD. > implement "wait" ioctl (wait for dma-buf->release): currently these are > DRM_XEN_ZCOPY_DUMB_FROM_REFS, DRM_XEN_ZCOPY_DUMB_TO_REFS and > DRM_XEN_ZCOPY_DUMB_WAIT_FREE > 1.2. Xen balloon driver [6] to allow allocating contiguous buffers (not > needed > by current hyper-dmabuf, but is a must for xen-zcopy use-cases) I think this needs clarifying. In which memory space do you need those regions to be contiguous? Do they need to be contiguous in host physical memory, or guest physical memory? If it's in guest memory space, isn't there any generic interface that you can use? If it's in host physical memory space, why do you need this buffer to be contiguous in host physical memory space? The IOMMU should hide all this. Thanks, Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver
On Wed, Apr 18, 2018 at 01:39:35PM +0300, Oleksandr Andrushchenko wrote: > On 04/18/2018 01:18 PM, Paul Durrant wrote: > > > -Original Message- > > > From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf > > > Of Roger Pau Monné > > > Sent: 18 April 2018 11:11 > > > To: Oleksandr Andrushchenko > > > Cc: jgr...@suse.com; Artem Mygaiev ; > > > Dongwon Kim ; airl...@linux.ie; > > > oleksandr_andrushche...@epam.com; linux-ker...@vger.kernel.org; dri- > > > de...@lists.freedesktop.org; Potrola, MateuszX > > > ; xen-de...@lists.xenproject.org; > > > daniel.vet...@intel.com; boris.ostrov...@oracle.com; Matt Roper > > > > > > Subject: Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy > > > helper DRM driver > > > > > > On Wed, Apr 18, 2018 at 11:01:12AM +0300, Oleksandr Andrushchenko > > > wrote: > > > > On 04/18/2018 10:35 AM, Roger Pau Monné wrote: > > > After speaking with Oleksandr on IRC, I think the main usage of the > > > gntdev extension is to: > > > > > > 1. Create a dma-buf from a set of grant references. > > > 2. Share dma-buf and get a list of grant references. > > > > > > I think this set of operations could be broken into: > > > > > > 1.1 Map grant references into user-space using the gntdev. > > > 1.2 Create a dma-buf out of a set of user-space virtual addresses. > > > > > > 2.1 Map a dma-buf into user-space. > > > 2.2 Get grefs out of the user-space addresses where the dma-buf is > > > mapped. > > > > > > So it seems like what's actually missing is a way to: > > > > > > - Create a dma-buf from a list of user-space virtual addresses. > > > - Allow to map a dma-buf into user-space, so it can then be used with > > > the gntdev. > > > > > > I think this is generic enough that it could be implemented by a > > > device not tied to Xen. AFAICT the hyper_dma guys also wanted > > > something similar to this. > Ok, so just to summarize, xen-zcopy/hyper-dmabuf as they are now, > are no go from your POV? My opinion is that there seems to be a more generic way to implement this, and thus I would prefer that one. > Instead, we have to make all that fancy stuff > with VAs <-> device-X and have that device-X driver live out of drivers/xen > as it is not a Xen specific driver? That would be my preference if feasible, simply because it can be reused by other use-cases that need to create dma-bufs in user-space. In any case I just knew about dma-bufs this morning, there might be things that I'm missing. Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver
On Wed, Apr 18, 2018 at 11:01:12AM +0300, Oleksandr Andrushchenko wrote: > On 04/18/2018 10:35 AM, Roger Pau Monné wrote: > > On Wed, Apr 18, 2018 at 09:38:39AM +0300, Oleksandr Andrushchenko wrote: > > > On 04/17/2018 11:57 PM, Dongwon Kim wrote: > > > > On Tue, Apr 17, 2018 at 09:59:28AM +0200, Daniel Vetter wrote: > > > > > On Mon, Apr 16, 2018 at 12:29:05PM -0700, Dongwon Kim wrote: > > > 3.2 Backend exports dma-buf to xen-front > > > > > > In this case Dom0 pages are shared with DomU. As before, DomU can only > > > write > > > to these pages, not any other page from Dom0, so it can be still > > > considered > > > safe. > > > But, the following must be considered (highlighted in xen-front's Kernel > > > documentation): > > > - If guest domain dies then pages/grants received from the backend > > > cannot > > > be claimed back - think of it as memory lost to Dom0 (won't be used > > > for > > > any > > > other guest) > > > - Misbehaving guest may send too many requests to the backend exhausting > > > its grant references and memory (consider this from security POV). As > > > the > > > backend runs in the trusted domain we also assume that it is trusted > > > as > > > well, > > > e.g. must take measures to prevent DDoS attacks. > > I cannot parse the above sentence: > > > > "As the backend runs in the trusted domain we also assume that it is > > trusted as well, e.g. must take measures to prevent DDoS attacks." > > > > What's the relation between being trusted and protecting from DoS > > attacks? > I mean that we trust the backend that it can prevent Dom0 > from crashing in case DomU's frontend misbehaves, e.g. > if the frontend sends too many memory requests etc. > > In any case, all? PV protocols are implemented with the frontend > > sharing pages to the backend, and I think there's a reason why this > > model is used, and it should continue to be used. > This is the first use-case above. But there are real-world > use-cases (embedded in my case) when physically contiguous memory > needs to be shared, one of the possible ways to achieve this is > to share contiguous memory from Dom0 to DomU (the second use-case above) > > Having to add logic in the backend to prevent such attacks means > > that: > > > > - We need more code in the backend, which increases complexity and > > chances of bugs. > > - Such code/logic could be wrong, thus allowing DoS. > You can live without this code at all, but this is then up to > backend which may make Dom0 down because of DomU's frontend doing evil > things IMO we should design protocols that do not allow such attacks instead of having to defend against them. > > > 4. xen-front/backend/xen-zcopy synchronization > > > > > > 4.1. As I already said in 2) all the inter VM communication happens > > > between > > > xen-front and the backend, xen-zcopy is NOT involved in that. > > > When xen-front wants to destroy a display buffer (dumb/dma-buf) it issues > > > a > > > XENDISPL_OP_DBUF_DESTROY command (opposite to XENDISPL_OP_DBUF_CREATE). > > > This call is synchronous, so xen-front expects that backend does free the > > > buffer pages on return. > > > > > > 4.2. Backend, on XENDISPL_OP_DBUF_DESTROY: > > > - closes all dumb handles/fd's of the buffer according to [3] > > > - issues DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE IOCTL to xen-zcopy to make > > > sure > > > the buffer is freed (think of it as it waits for dma-buf->release > > > callback) > > So this zcopy thing keeps some kind of track of the memory usage? Why > > can't the user-space backend keep track of the buffer usage? > Because there is no dma-buf UAPI which allows to track the buffer life cycle > (e.g. wait until dma-buf's .release callback is called) > > > - replies to xen-front that the buffer can be destroyed. > > > This way deletion of the buffer happens synchronously on both Dom0 and > > > DomU > > > sides. In case if DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE returns with time-out > > > error > > > (BTW, wait time is a parameter of this IOCTL), Xen will defer grant > > > reference > > > removal and will retry later until those are free. > > > > > > Hope this helps understand how buffers are synchronously deleted in case > > > of xen-zcopy with a single protocol command. >
Re: [Xen-devel] [RFC PATCH v2 2/9] hyper_dmabuf: architecture specification and reference guide
On Tue, Feb 13, 2018 at 05:50:01PM -0800, Dongwon Kim wrote: > Reference document for hyper_DMABUF driver > > Documentation/hyper-dmabuf-sharing.txt This should likely be patch 1 in order for reviewers to have the appropriate context. > > Signed-off-by: Dongwon Kim > --- > Documentation/hyper-dmabuf-sharing.txt | 734 > + > 1 file changed, 734 insertions(+) > create mode 100644 Documentation/hyper-dmabuf-sharing.txt > > diff --git a/Documentation/hyper-dmabuf-sharing.txt > b/Documentation/hyper-dmabuf-sharing.txt > new file mode 100644 > index ..928e411931e3 > --- /dev/null > +++ b/Documentation/hyper-dmabuf-sharing.txt > @@ -0,0 +1,734 @@ > +Linux Hyper DMABUF Driver > + > +-- > +Section 1. Overview > +-- > + > +Hyper_DMABUF driver is a Linux device driver running on multiple Virtual > +achines (VMs), which expands DMA-BUF sharing capability to the VM environment > +where multiple different OS instances need to share same physical data > without > +data-copy across VMs. > + > +To share a DMA_BUF across VMs, an instance of the Hyper_DMABUF drv on the > +exporting VM (so called, “exporter”) imports a local DMA_BUF from the > original > +producer of the buffer, The usage of export and import in the above sentence makes it almost impossible to understand. > then re-exports it with an unique ID, hyper_dmabuf_id > +for the buffer to the importing VM (so called, “importer”). And this is even worse. Maybe it would help to have some kind of flow diagram of all this import/export operations, but please read below. > + > +Another instance of the Hyper_DMABUF driver on importer registers > +a hyper_dmabuf_id together with reference information for the shared physical > +pages associated with the DMA_BUF to its database when the export happens. > + > +The actual mapping of the DMA_BUF on the importer’s side is done by > +the Hyper_DMABUF driver when user space issues the IOCTL command to access > +the shared DMA_BUF. The Hyper_DMABUF driver works as both an importing and > +exporting driver as is, that is, no special configuration is required. > +Consequently, only a single module per VM is needed to enable cross-VM > DMA_BUF > +exchange. IMHO I need a more generic view of the problem you are trying to solve in the overview section. I've read the full overview, and I still have no idea why you need all this. I think the overview should contain at least: 1. A description of the problem you are trying to solve. 2. A high level description of the proposed solution. 3. How the proposed solution deals with the problem described in 1. This overview is not useful for people that don't know which problem you are trying to solve, like myself. Thanks, Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Xen-devel] [PATCH 1/9] drm/xen-front: Introduce Xen para-virtualized frontend driver
On Wed, Feb 21, 2018 at 10:03:34AM +0200, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko > > Introduce skeleton of the para-virtualized Xen display > frontend driver. This patch only adds required > essential stubs. > > Signed-off-by: Oleksandr Andrushchenko > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile| 1 + > drivers/gpu/drm/xen/Kconfig | 17 > drivers/gpu/drm/xen/Makefile| 5 +++ > drivers/gpu/drm/xen/xen_drm_front.c | 83 > + > 5 files changed, 108 insertions(+) > create mode 100644 drivers/gpu/drm/xen/Kconfig > create mode 100644 drivers/gpu/drm/xen/Makefile > create mode 100644 drivers/gpu/drm/xen/xen_drm_front.c > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index deeefa7a1773..757825ac60df 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -289,6 +289,8 @@ source "drivers/gpu/drm/pl111/Kconfig" > > source "drivers/gpu/drm/tve200/Kconfig" > > +source "drivers/gpu/drm/xen/Kconfig" > + > # Keep legacy drivers last > > menuconfig DRM_LEGACY > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 50093ff4479b..9d66657ea117 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -103,3 +103,4 @@ obj-$(CONFIG_DRM_MXSFB) += mxsfb/ > obj-$(CONFIG_DRM_TINYDRM) += tinydrm/ > obj-$(CONFIG_DRM_PL111) += pl111/ > obj-$(CONFIG_DRM_TVE200) += tve200/ > +obj-$(CONFIG_DRM_XEN) += xen/ > diff --git a/drivers/gpu/drm/xen/Kconfig b/drivers/gpu/drm/xen/Kconfig > new file mode 100644 > index ..4cca160782ab > --- /dev/null > +++ b/drivers/gpu/drm/xen/Kconfig > @@ -0,0 +1,17 @@ > +config DRM_XEN > + bool "DRM Support for Xen guest OS" > + depends on XEN > + help > + Choose this option if you want to enable DRM support > + for Xen. > + > +config DRM_XEN_FRONTEND > + tristate "Para-virtualized frontend driver for Xen guest OS" > + depends on DRM_XEN > + depends on DRM > + select DRM_KMS_HELPER > + select VIDEOMODE_HELPERS > + select XEN_XENBUS_FRONTEND > + help > + Choose this option if you want to enable a para-virtualized > + frontend DRM/KMS driver for Xen guest OSes. > diff --git a/drivers/gpu/drm/xen/Makefile b/drivers/gpu/drm/xen/Makefile > new file mode 100644 > index ..967074d348f6 > --- /dev/null > +++ b/drivers/gpu/drm/xen/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +drm_xen_front-objs := xen_drm_front.o > + > +obj-$(CONFIG_DRM_XEN_FRONTEND) += drm_xen_front.o > diff --git a/drivers/gpu/drm/xen/xen_drm_front.c > b/drivers/gpu/drm/xen/xen_drm_front.c > new file mode 100644 > index ..fd372fb464a1 > --- /dev/null > +++ b/drivers/gpu/drm/xen/xen_drm_front.c > @@ -0,0 +1,83 @@ > +/* > + * Xen para-virtual DRM device > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. Most Xen drivers in Linux use a dual GPL/BSD license, so that they can be imported into other non GPL OSes: This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License version 2 as published by the Free Software Foundation; or, when distributed separately from the Linux kernel or incorporated into other software packages, subject to the following license: Permission is hereby granted, free of charge, to any person obtaining a copy of this source file (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. IMO it would be good to release this driver under the same license, so it can be incorporated into other OSes. Thanks, R
Re: [Xen-devel] [PATCH 1/9] drm/xen-front: Introduce Xen para-virtualized frontend driver
On Wed, Feb 21, 2018 at 11:42:23AM +0200, Oleksandr Andrushchenko wrote: > On 02/21/2018 11:17 AM, Roger Pau Monné wrote: > > On Wed, Feb 21, 2018 at 10:03:34AM +0200, Oleksandr Andrushchenko wrote: > > > --- /dev/null > > > +++ b/drivers/gpu/drm/xen/xen_drm_front.c > > > @@ -0,0 +1,83 @@ > > > +/* > > > + * Xen para-virtual DRM device > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License as published by > > > + * the Free Software Foundation; either version 2 of the License, or > > > + * (at your option) any later version. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > Most Xen drivers in Linux use a dual GPL/BSD license, so that they can > > be imported into other non GPL OSes: > > > > This program is free software; you can redistribute it and/or > > modify it under the terms of the GNU General Public License version 2 > > as published by the Free Software Foundation; or, when distributed > > separately from the Linux kernel or incorporated into other > > software packages, subject to the following license: > > > > Permission is hereby granted, free of charge, to any person obtaining a copy > > of this source file (the "Software"), to deal in the Software without > > restriction, including without limitation the rights to use, copy, modify, > > merge, publish, distribute, sublicense, and/or sell copies of the Software, > > and to permit persons to whom the Software is furnished to do so, subject to > > the following conditions: > > > > The above copyright notice and this permission notice shall be included in > > all copies or substantial portions of the Software. > > > > THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE > > AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > IN THE SOFTWARE. > > > > IMO it would be good to release this driver under the same license, so > > it can be incorporated into other OSes. > I am in any way expert in licensing, but the above seems to be > /* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ > At least this is what I see at [1] for MIT. > Could you please tell which license(s) as listed at [1] > would be appropriate for Xen drivers in terms of how it is > expected to appear in the kernel code, e.g. expected > SPDX-License-Identifier? I would be fine with anything MIT/BSD-*/Apache-* like. In the Xen community we have generally done dual GPL-2.0 MIT, so your proposed tag looks fine IMO (I would also personally be fine with MIT/BSD-*/Apache-* only). The point is that it would be good to have the code under a more permissive license so it can be integrated into non GPL OSes in the future if needed, and that your code could be used as a reference for that. Thanks, Roger. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote:
> Straightforward conversion to the new helper, except due to the lack
> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in
> certain cases in the future.
>
> Signed-off-by: Logan Gunthorpe
> Cc: Boris Ostrovsky
> Cc: Juergen Gross
> Cc: Konrad Rzeszutek Wilk
> Cc: "Roger Pau Monné"
> ---
> drivers/block/xen-blkfront.c | 20 +++-
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 3945963..ed62175 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct
> blkfront_ring_info *ri
> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
>
> if (setup.need_copy) {
> - setup.bvec_off = sg->offset;
> - setup.bvec_data = kmap_atomic(sg_page(sg));
> + setup.bvec_off = 0;
> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC |
> + SG_MAP_MUST_NOT_FAIL);
I assume that sg_map already adds sg->offset to the address?
Also wondering whether we can get rid of bvec_off and just increment bvec_data,
adding Julien who IIRC added this code.
> }
>
> gnttab_foreach_grant_in_range(sg_page(sg),
> @@ -827,7 +828,7 @@ static int blkif_queue_rw_req(struct request *req, struct
> blkfront_ring_info *ri
> &setup);
>
> if (setup.need_copy)
> - kunmap_atomic(setup.bvec_data);
> + sg_unmap(sg, setup.bvec_data, 0, SG_KMAP_ATOMIC);
> }
> if (setup.segments)
> kunmap_atomic(setup.segments);
> @@ -1053,7 +1054,7 @@ static int xen_translate_vdev(int vdevice, int *minor,
> unsigned int *offset)
> case XEN_SCSI_DISK5_MAJOR:
> case XEN_SCSI_DISK6_MAJOR:
> case XEN_SCSI_DISK7_MAJOR:
> - *offset = (*minor / PARTS_PER_DISK) +
> + *offset = (*minor / PARTS_PER_DISK) +
> ((major - XEN_SCSI_DISK1_MAJOR + 1) * 16) +
> EMULATED_SD_DISK_NAME_OFFSET;
> *minor = *minor +
> @@ -1068,7 +1069,7 @@ static int xen_translate_vdev(int vdevice, int *minor,
> unsigned int *offset)
> case XEN_SCSI_DISK13_MAJOR:
> case XEN_SCSI_DISK14_MAJOR:
> case XEN_SCSI_DISK15_MAJOR:
> - *offset = (*minor / PARTS_PER_DISK) +
> + *offset = (*minor / PARTS_PER_DISK) +
> ((major - XEN_SCSI_DISK8_MAJOR + 8) * 16) +
> EMULATED_SD_DISK_NAME_OFFSET;
> *minor = *minor +
> @@ -1119,7 +1120,7 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
> if (!VDEV_IS_EXTENDED(info->vdevice)) {
> err = xen_translate_vdev(info->vdevice, &minor, &offset);
> if (err)
> - return err;
> + return err;
Cosmetic changes should go in a separate patch please.
Roger.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
