Re: [RESUBMIT][PATCH] x86/mm: Fix PAT bit missing from page protection modify mask

[Juergen Gross]

On 02.06.23 16:48, Juergen Gross wrote:

On 02.06.23 16:43, Borislav Petkov wrote:

On Thu, Jun 01, 2023 at 10:47:39AM +0200, Juergen Gross wrote:

As described in the commit message, this only works on bare metal due to the
PAT bit not being needed for WC mappings.

Making this patch Xen specific would try to cure the symptoms without fixing
the underlying problem: _PAGE_PAT should be regarded the same way as the bits
for caching mode (_PAGE_CHG_MASK).


So why isn't _PAGE_PAT part of _PAGE_CHG_MASK?


This would result in problems for large pages: _PAGE_PSE is at the same
position as _PAGE_PAT (large pages are using _PAGE_PAT_LARGE instead).

Yes, x86 ABI is a mess.


Oh, wait: I originally thought _PAGE_CHG_MASK would be used for large pages,
too. There is _HPAGE_CHG_MASK for that purpose.

So adding _PAGE_PAT to _PAGE_CHG_MASK and _PAGE_PAT_LARGE to _HPAGE_CHG_MASK
should do the job. At least I hope so.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RESUBMIT][PATCH] x86/mm: Fix PAT bit missing from page protection modify mask

[Juergen Gross]

On 02.06.23 16:43, Borislav Petkov wrote:

On Thu, Jun 01, 2023 at 10:47:39AM +0200, Juergen Gross wrote:

As described in the commit message, this only works on bare metal due to the
PAT bit not being needed for WC mappings.

Making this patch Xen specific would try to cure the symptoms without fixing
the underlying problem: _PAGE_PAT should be regarded the same way as the bits
for caching mode (_PAGE_CHG_MASK).


So why isn't _PAGE_PAT part of _PAGE_CHG_MASK?


This would result in problems for large pages: _PAGE_PSE is at the same
position as _PAGE_PAT (large pages are using _PAGE_PAT_LARGE instead).

Yes, x86 ABI is a mess.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RESUBMIT][PATCH] x86/mm: Fix PAT bit missing from page protection modify mask

[Juergen Gross]

On 31.05.23 20:14, Borislav Petkov wrote:

On Fri, May 19, 2023 at 08:36:34PM +0200, Janusz Krzysztofik wrote:

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 15ae4d6ba4768..56466afd04307 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -654,8 +654,10 @@ static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
  #define pgprot_modify pgprot_modify
  static inline pgprot_t pgprot_modify(pgprot_t oldprot, pgprot_t newprot)
  {
-   pgprotval_t preservebits = pgprot_val(oldprot) & _PAGE_CHG_MASK;
-   pgprotval_t addbits = pgprot_val(newprot) & ~_PAGE_CHG_MASK;
+   unsigned long mask = _PAGE_CHG_MASK | _PAGE_CACHE_MASK;
+
+   pgprotval_t preservebits = pgprot_val(oldprot) & mask;
+   pgprotval_t addbits = pgprot_val(newprot) & ~mask;
return __pgprot(preservebits | addbits);
  }
  
--


This certainly needs Jürgen and he's on CC already, moving him to To:.

Also, why isn't this a Xen-specific fix but you're keeping _PAGE_PAT for
baremetal too, i.e., modifying the generic function?



As described in the commit message, this only works on bare metal due to the
PAT bit not being needed for WC mappings.

Making this patch Xen specific would try to cure the symptoms without fixing
the underlying problem: _PAGE_PAT should be regarded the same way as the bits
for caching mode (_PAGE_CHG_MASK).

In case a WP or WT mapped memory area would be mmap()-ed on bare metal, the
result would be a WC or UC mapped memory area in userland. This isn't as
problematic as the case under Xen, but it still results in worse performance
than necessary.

IOW:

Acked-by: Juergen Gross 


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RFC PATCH] x86/mm: Fix PAT bit missing from page protection modify mask

[Juergen Gross]

On 24.04.23 14:35, Janusz Krzysztofik wrote:

Visible glitches have been observed when running graphics applications on
Linux under Xen hypervisor.  Those observations have been confirmed with
failures from kms_pwrite_crc Intel GPU test that verifies data coherency
of DRM frame buffer objects using hardware CRC checksums calculated by
display controllers, exposed to userspace via debugfs.  Affected
processing paths have then been identified with new test variants that
mmap the objects using different methods and caching modes.

When running as a Xen PV guest, Linux uses Xen provided PAT configuration
which is different from its native one.  In particular, Xen specific PTE
encoding of write-combining caching, likely used by graphics applications,
differs from the Linux default one found among statically defined minimal
set of supported modes.  Since Xen defines PTE encoding of the WC mode as
_PAGE_PAT, it no longer belongs to the minimal set, depends on correct
handling of _PAGE_PAT bit, and can be mismatched with write-back caching.

When a user calls mmap() for a DRM buffer object, DRM device specific
.mmap file operation, called from mmap_region(), takes care of setting PTE
encoding bits in a vm_page_prot field of an associated virtual memory area
structure.  Unfortunately, _PAGE_PAT bit is not preserved when the vma's
.vm_flags are then applied to .vm_page_prot via vm_set_page_prot().  Bits
to be preserved are determined with _PAGE_CHG_MASK symbol that doesn't
cover _PAGE_PAT.  As a consequence, WB caching is requested instead of WC
when running under Xen (also, WP is silently changed to WT, and UC
downgraded to UC_MINUS).  When running on bare metal, WC is not affected,
but WP and WT extra modes are unintentionally replaced with WC and UC,
respectively.

WP and WT modes, encoded with _PAGE_PAT bit set, were introduced by commit
281d4078bec3 ("x86: Make page cache mode a real type").  Care was taken
to extend _PAGE_CACHE_MASK symbol with that additional bit, but that
symbol has never been used for identification of bits preserved when
applying page protection flags.  Support for all cache modes under Xen,
including the problematic WC mode, was then introduced by commit
47591df50512 ("xen: Support Xen pv-domains using PAT").

Extend bitmask used by pgprot_modify() for selecting bits to be preserved
with _PAGE_PAT bit.  However, since that bit can be reused as _PAGE_PSE,
and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely
intentionally defined with that bit not set, keep that symbol unchanged.


Hmm, I wonder whether pte_mkhuge() shouldn't just set _PAGE_PSE, but use
pgprot_4k_2_large() before doing so.

OTOH a use case like in remove_migration_pte(), where pte_mkhuge() is
directly followed by a call of arch_make_huge_pte(), which in turn is
calling pte_mkhuge() again, would set _always_ the PAT bit.

When running as a Xen PV guest this doesn't matter at all, as large or
huge pages aren't supported there. So clearly something for the MM
maintainers. :-)


Juergen

P.S.: Janusz, nice catch! The QubesOS folks who reported the problem
  originally will test your patch under Xen soon.


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Juergen Gross]

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.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Juergen Gross]

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.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: Issue on unmap_grant_pages before releasing dmabuf

[Juergen Gross]

On 19.10.22 12:43, Oleksii Moisieiev wrote:

Greetings.

I need your advise about the problem I'm facing right now:
I'm working on the new type of dmabuf export implementation. This
is going to be new ioctl to the gntdev and it's purpose is to use
external buffer, provided by file descriptor as the backing storage
during export to grant refs.
Few words about the functionality I'm working on right now:
My setup is based on IMX8QM (please see PS below if you need
configuration details)

When using dma-buf exporter to create dma-buf with backing storage and
map it to the grant refs, provided from the domain, we've met a problem,
that several HW (i.MX8 gpu in our case) do not support external buffer
and requires backing storage to be created using it's native tools
(eglCreateImageKHR returns EGL_NO_IMAGE_KHR for buffers, which were not
created using gbm_bo_create).
That's why new ioctls were added to be able to pass existing dma-buffer
fd as input parameter and use it as backing storage to export to refs.
Kernel version on IMX8QM board is 5.10.72 and itworks fine on this kernel
version.

New ioctls source code can be found here:
  
https://github.com/oleksiimoisieiev/linux/tree/gntdev_map_buf_upstr_for-linus-6.1_2
 
Now regarding the problem I've met when rebased those code on master:

On my test stand I use DRM_IOCTL_MODE_CREATE_DUMB/DRM_IOCTL_MODE_DESTROY_DUMB 
ioctls
to allocate buffer and I'm observing the following backtrace on 
DRM_IOCTL_MODE_DESTROY_DUMB:

Unable to handle kernel paging request at virtual address 00038798
Mem abort info:
   ESR = 0x9605
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x05: level 1 translation fault
Data abort info:
   ISV = 0, ISS = 0x0005
   CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=6df98000
[00038798] pgd=080064f4f003, p4d=080064f4f003, 
pud=
Internal error: Oops: 9605 [#1] PREEMPT SMP
Modules linked in: xen_pciback overlay crct10dif_ce ip_tables x_tables ipv6
PU: 0 PID: 34 Comm: kworker/0:1 Not tainted 6.0.0 #85
Hardware name: linux,dummy-virt (DT)
Workqueue: events virtio_gpu_dequeue_ctrl_func
pstate: 00c5 (nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : check_move_unevictable_folios+0xb8/0x4d0
lr : check_move_unevictable_folios+0xb4/0x4d0
sp : 881a3ad0
x29: 881a3ad0 x28: 03856ac98800 x27: 
x26: de7b168ee9d8 x25: 03856ae26008 x24: 
x23: de7b1758d6c0 x22: 0001 x21: 881a3b68
x20: 0001 x19: fc0e15935040 x18: 
x17: 250a68e3d000 x16: 0012 x15: 8000881a38d7
x14:  x13: de7b175a3150 x12: 2c55
x11: 0ec7 x10: de7b176113f8 x9 : de7b175a3150
x8 : 00014ec7 x7 : de7b175fb150 x6 : 881a3b70
x5 : 0001 x4 :  x3 : 03856ac98850
x2 :  x1 :  x0 : 00038700
Call trace:
  check_move_unevictable_folios+0xb8/0x4d0
  check_move_unevictable_pages+0x8c/0x110
  drm_gem_put_pages+0x118/0x198
  drm_gem_shmem_put_pages_locked+0x4c/0x70
  drm_gem_shmem_unpin+0x30/0x50
  virtio_gpu_cleanup_object+0x84/0x130
  virtio_gpu_cmd_unref_cb+0x18/0x2c
  virtio_gpu_dequeue_ctrl_func+0x124/0x290
  process_one_work+0x1d0/0x320
  worker_thread+0x14c/0x444
  kthread+0x10c/0x110
  ret_from_fork+0x10/0x20
Code: 97fc3fe1 aa1303e0 94003ac7 b480 (f9404c00)
---[ end trace  ]---

After some investigation I think I've found the cause of the problem:
This is the functionality, added in commit 
3f9f1c67572f5e5e6dc84216d48d1480f3c4fcf6 which
introduces safe mechanism to unmap grant pages which is waiting until 
page_count(page) = 1
before doing unmap.
The problem is that DRM_IOCTL_MODE_CREATE_DUMB creates buffer where 
page_count(page) = 2.

On my QEMU test stand I'm using Xen 4.16 (aarch64) with debian based Dom0 + 
DomU on the latest
kernels.
I've created some apps for testing:
The first one is to allocate grant refs on DomU:
https://github.com/oleksiimoisieiev/linux/tree/gntdev_map_buf_upstr_for-linus-6.1_2
The name is test.ko and it can be built using command:
cd ./test; make
NOTE: makefile expects kernel build to be present on ../../test-build directory.

It should be run on DomU using command:
insmod test.ko; echo "create" > /sys/kernel/etx_sysfs/etx_value

Result will be the following:
[  126.104903] test: loading out-of-tree module taints kernel.
[  126.150586] Sysfs - Write!!!
[  126.150773] create
[  126.150773]
[  126.150888] Hello, World!
[  126.151203] Hello, World!
[  126.151324] gref 301
[  126.151376] addr 0883d000
[  126.151431] gref 302
[  126.151454] addr 0883e000
[  126.151478] gref 303
[  126.151497] addr 0883f000
[  126.151525] gref 304
[  126.151546] addr 0884
[  126.151573] gref 305
[  126.151593] addr 08841000

The second 

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Juergen Gross]

On 18.10.22 16:33, Christoph Hellwig wrote:

On Tue, Oct 18, 2022 at 04:21:43PM +0200, Jan Beulich wrote:

Leaving the "i915 abuses" part aside (because I can't tell what exactly the
abuse is), but assuming that "can't cope with bounce buffering" means they
don't actually use the allocated buffers, I'd suggest this:


Except for one odd place i915 never uses dma_alloc_* but always allocates
memory itself and then maps it, but then treats it as if it was a
dma_alloc_coherent allocations, that is never does ownership changes.


I've dropped the TDX related remark because I don't think it's meaningful
for PV guests.


This remark is for TDX in general, not Xen related.  With TDX and other
confidentatial computing schemes, all DMA must be bounce buffered, and
all drivers skipping dma_sync* calls are broken.


Otoh I've left the "abuses ignores" word sequence as is, no
matter that it reads odd to me. Plus, as hinted at before, I'm not
convinced the IS_ENABLED() use is actually necessary or warranted here.


If we don't need the IS_ENABLED is not needed I'm all for dropping it.
But unless I misread the code, on arm/arm64 even PV guests are 1:1
mapped so that all Linux physically contigous memory also is Xen
contigous, so we don't need the hack.


There are no PV guests on arm/arm64.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 1/5] xen: add "not_essential" flag to struct xenbus_driver

[Juergen Gross]

On 22.10.21 11:28, Andrew Cooper wrote:

On 22/10/2021 07:47, Juergen Gross wrote:

When booting the xenbus driver will wait for PV devices to have
connected to their backends before continuing. The timeout is different
between essential and non-essential devices.

Non-essential devices are identified by their nodenames directly in the
xenbus driver, which requires to update this list in case a new device
type being non-essential is added (this was missed for several types
in the past).

In order to avoid this problem, add a "not_essential" flag to struct
xenbus_driver which can be set to "true" by the respective frontend.

Set this flag for the frontends currently regarded to be not essential
(vkbs and vfb) and use it for testing in the xenbus driver.

Signed-off-by: Juergen Gross 


Wouldn't it be better to annotate essential?  That way, when new misc
drivers come along, they don't by default block boot.


It isn't as if new drivers would "block boot". Normally the short
timeout for all drivers of 30 seconds is more than enough for all of
them.

I'm a little bit hesitant to have a kind of "white listing" essential
drivers, as there might be different views which drivers should have
that flag. Doing this the other way round is easier: in case of
disagreement such a patch just wouldn't go in, not breaking anything
in that case.

Additionally there might be out-of-tree PV drivers, which could be
hit by not being flagged to be essential. With the not_essential flag
the situation wouldn't change for such a driver.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 0/5] xen: cleanup detection of non-essential pv devices

[Juergen Gross]

On 22.10.21 09:24, Jan Beulich wrote:

On 22.10.2021 08:47, Juergen Gross wrote:

Today the non-essential pv devices are hard coded in the xenbus driver
and this list is lacking multiple entries.

This series reworks the detection logic of non-essential devices by
adding a flag for that purpose to struct xenbus_driver.


I'm wondering whether it wouldn't better be the other way around: The
(hopefully few) essential ones get flagged, thus also making it more
prominent during patch review that a flag gets added (and justification
provided), instead of having to spot the lack of a flag getting set.


Not flagging a non-essential one is less problematic than not flagging
an essential driver IMO.

For some drivers I'm on the edge, BTW. The pv 9pfs driver ought to be
non-essential in most cases, but there might be use cases where it is
needed, so I didn't set its non_essential flag.

Same applies to pv-usb and maybe pv-scsi, while pv-tpm probably really
is essential.

With the current series I'm ending up with 6 non-essential drivers and
6 essential ones, so either way needs the same number of drivers
modified.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH 2/5] xen: flag xen_drm_front to be not essential for system boot

[Juergen Gross]

Similar to the virtual frame buffer (vfb) the pv display driver is not
essential for booting the system. Set the respective flag.

Signed-off-by: Juergen Gross 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 9f14d99c763c..bc7605324db3 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -773,6 +773,7 @@ static struct xenbus_driver xen_driver = {
.probe = xen_drv_probe,
.remove = xen_drv_remove,
.otherend_changed = displback_changed,
+   .not_essential = true,
 };
 
 static int __init xen_drv_init(void)
-- 
2.26.2

[PATCH 1/5] xen: add "not_essential" flag to struct xenbus_driver

[Juergen Gross]

When booting the xenbus driver will wait for PV devices to have
connected to their backends before continuing. The timeout is different
between essential and non-essential devices.

Non-essential devices are identified by their nodenames directly in the
xenbus driver, which requires to update this list in case a new device
type being non-essential is added (this was missed for several types
in the past).

In order to avoid this problem, add a "not_essential" flag to struct
xenbus_driver which can be set to "true" by the respective frontend.

Set this flag for the frontends currently regarded to be not essential
(vkbs and vfb) and use it for testing in the xenbus driver.

Signed-off-by: Juergen Gross 
---
 drivers/input/misc/xen-kbdfront.c  |  1 +
 drivers/video/fbdev/xen-fbfront.c  |  1 +
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
 include/xen/xenbus.h   |  1 +
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c 
b/drivers/input/misc/xen-kbdfront.c
index 4ff5cd2a6d8d..3d17a0b3fe51 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -542,6 +542,7 @@ static struct xenbus_driver xenkbd_driver = {
.remove = xenkbd_remove,
.resume = xenkbd_resume,
.otherend_changed = xenkbd_backend_changed,
+   .not_essential = true,
 };
 
 static int __init xenkbd_init(void)
diff --git a/drivers/video/fbdev/xen-fbfront.c 
b/drivers/video/fbdev/xen-fbfront.c
index 5ec51445bee8..6826f986da43 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -695,6 +695,7 @@ static struct xenbus_driver xenfb_driver = {
.remove = xenfb_remove,
.resume = xenfb_resume,
.otherend_changed = xenfb_backend_changed,
+   .not_essential = true,
 };
 
 static int __init xenfb_init(void)
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c 
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 480944606a3c..07b010a68fcf 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -211,19 +211,11 @@ static int is_device_connecting(struct device *dev, void 
*data, bool ignore_none
if (drv && (dev->driver != drv))
return 0;
 
-   if (ignore_nonessential) {
-   /* With older QEMU, for PVonHVM guests the guest config files
-* could contain: vfb = [ 'vnc=1, vnclisten=0.0.0.0']
-* which is nonsensical as there is no PV FB (there can be
-* a PVKB) running as HVM guest. */
+   xendrv = to_xenbus_driver(dev->driver);
 
-   if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
-   return 0;
+   if (ignore_nonessential && xendrv->not_essential)
+   return 0;
 
-   if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
-   return 0;
-   }
-   xendrv = to_xenbus_driver(dev->driver);
return (xendev->state < XenbusStateConnected ||
(xendev->state == XenbusStateConnected &&
 xendrv->is_ready && !xendrv->is_ready(xendev)));
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index b94074c82772..b13eb86395e0 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -112,6 +112,7 @@ struct xenbus_driver {
const char *name;   /* defaults to ids[0].devicetype */
const struct xenbus_device_id *ids;
bool allow_rebind; /* avoid setting xenstore closed during remove */
+   bool not_essential; /* is not mandatory for boot progress */
int (*probe)(struct xenbus_device *dev,
 const struct xenbus_device_id *id);
void (*otherend_changed)(struct xenbus_device *dev,
-- 
2.26.2

[PATCH 0/5] xen: cleanup detection of non-essential pv devices

[Juergen Gross]

Today the non-essential pv devices are hard coded in the xenbus driver
and this list is lacking multiple entries.

This series reworks the detection logic of non-essential devices by
adding a flag for that purpose to struct xenbus_driver.

Juergen Gross (5):
  xen: add "not_essential" flag to struct xenbus_driver
  xen: flag xen_drm_front to be not essential for system boot
  xen: flag hvc_xen to be not essential for system boot
  xen: flag pvcalls-front to be not essential for system boot
  xen: flag xen_snd_front to be not essential for system boot

 drivers/gpu/drm/xen/xen_drm_front.c|  1 +
 drivers/input/misc/xen-kbdfront.c  |  1 +
 drivers/tty/hvc/hvc_xen.c  |  1 +
 drivers/video/fbdev/xen-fbfront.c  |  1 +
 drivers/xen/pvcalls-front.c|  1 +
 drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
 include/xen/xenbus.h   |  1 +
 sound/xen/xen_snd_front.c  |  1 +
 8 files changed, 10 insertions(+), 11 deletions(-)

-- 
2.26.2

[PATCH v2] efi: avoid error message when booting under Xen

[Juergen Gross]

efifb_probe() will issue an error message in case the kernel is booted
as Xen dom0 from UEFI as EFI_MEMMAP won't be set in this case. Avoid
that message by calling efi_mem_desc_lookup() only if EFI_MEMMAP is set.

Fixes: 38ac0287b7f4 ("fbdev/efifb: Honour UEFI memory map attributes when 
mapping the FB")
Signed-off-by: Juergen Gross 
---
 drivers/video/fbdev/efifb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 65491ae74808..e57c00824965 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -453,7 +453,7 @@ static int efifb_probe(struct platform_device *dev)
info->apertures->ranges[0].base = efifb_fix.smem_start;
info->apertures->ranges[0].size = size_remap;
 
-   if (efi_enabled(EFI_BOOT) &&
+   if (efi_enabled(EFI_MEMMAP) &&
!efi_mem_desc_lookup(efifb_fix.smem_start, )) {
if ((efifb_fix.smem_start + efifb_fix.smem_len) >
(md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT))) {
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] efi: avoid error message when booting under Xen

[Juergen Gross]

efifb_probe() will issue an error message in case the kernel is booted
as Xen dom0 from UEFI as EFI_MEMMAP won't be set in this case. Avoid
that message by calling efi_mem_desc_lookup() only if EFI_PARAVIRT
isn't set.

Fixes: 38ac0287b7f4 ("fbdev/efifb: Honour UEFI memory map attributes when 
mapping the FB")
Signed-off-by: Juergen Gross 
---
 drivers/video/fbdev/efifb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 65491ae74808..f5eccd1373e9 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -453,7 +453,7 @@ static int efifb_probe(struct platform_device *dev)
info->apertures->ranges[0].base = efifb_fix.smem_start;
info->apertures->ranges[0].size = size_remap;
 
-   if (efi_enabled(EFI_BOOT) &&
+   if (efi_enabled(EFI_BOOT) && !efi_enabled(EFI_PARAVIRT) &&
!efi_mem_desc_lookup(efifb_fix.smem_start, )) {
if ((efifb_fix.smem_start + efifb_fix.smem_len) >
(md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT))) {
-- 
2.26.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 20/34] xen: convert put_page() to put_user_page*()

[Juergen Gross]

On 05.08.19 00:49, john.hubb...@gmail.com wrote:

From: John Hubbard 

For pages that were retained via get_user_pages*(), release those pages
via the new put_user_page*() routines, instead of via put_page() or
release_pages().

This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
("mm: introduce put_user_page*(), placeholder versions").

This also handles pages[i] == NULL cases, thanks to an approach
that is actually written by Juergen Gross.

Signed-off-by: Juergen Gross 
Signed-off-by: John Hubbard 

Cc: Boris Ostrovsky 
Cc: xen-de...@lists.xenproject.org
---

Hi Juergen,

Say, this is *exactly* what you proposed in your gup.patch, so
I've speculatively added your Signed-off-by above, but need your
approval before that's final. Let me know please...


Yes, that's fine with me.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 20/34] xen: convert put_page() to put_user_page*()

[Juergen Gross]

On 02.08.19 07:48, John Hubbard wrote:

On 8/1/19 9:36 PM, Juergen Gross wrote:

On 02.08.19 04:19, john.hubb...@gmail.com wrote:

From: John Hubbard 

...

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..29e461dbee2d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,15 +611,10 @@ static int lock_pages(
  static void unlock_pages(struct page *pages[], unsigned int nr_pages)
  {
-    unsigned int i;
-
  if (!pages)
  return;
-    for (i = 0; i < nr_pages; i++) {
-    if (pages[i])
-    put_page(pages[i]);
-    }
+    put_user_pages(pages, nr_pages);


You are not handling the case where pages[i] is NULL here. Or am I
missing a pending patch to put_user_pages() here?



Hi Juergen,

You are correct--this no longer handles the cases where pages[i]
is NULL. It's intentional, though possibly wrong. :)

I see that I should have added my standard blurb to this
commit description. I missed this one, but some of the other patches
have it. It makes the following, possibly incorrect claim:

"This changes the release code slightly, because each page slot in the
page_list[] array is no longer checked for NULL. However, that check
was wrong anyway, because the get_user_pages() pattern of usage here
never allowed for NULL entries within a range of pinned pages."

The way I've seen these page arrays used with get_user_pages(),
things are either done single page, or with a contiguous range. So
unless I'm missing a case where someone is either

a) releasing individual pages within a range (and thus likely messing
up their count of pages they have), or

b) allocating two gup ranges within the same pages[] array, with a
gap between the allocations,

...then it should be correct. If so, then I'll add the above blurb
to this patch's commit description.

If that's not the case (both here, and in 3 or 4 other patches in this
series, then as you said, I should add NULL checks to put_user_pages()
and put_user_pages_dirty_lock().


In this case it is not correct, but can easily be handled. The NULL case
can occur only in an error case with the pages array filled partially or
not at all.

I'd prefer something like the attached patch here.


Juergen
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..12bd3154126d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch(
 
 static int lock_pages(
 	struct privcmd_dm_op_buf kbufs[], unsigned int num,
-	struct page *pages[], unsigned int nr_pages)
+	struct page *pages[], unsigned int *nr_pages)
 {
-	unsigned int i;
+	unsigned int i, free = *nr_pages;
 
+	*nr_pages = 0;
 	for (i = 0; i < num; i++) {
 		unsigned int requested;
 		int pinned;
@@ -593,35 +594,22 @@ static int lock_pages(
 		requested = DIV_ROUND_UP(
 			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
 			PAGE_SIZE);
-		if (requested > nr_pages)
+		if (requested > free)
 			return -ENOSPC;
 
 		pinned = get_user_pages_fast(
 			(unsigned long) kbufs[i].uptr,
-			requested, FOLL_WRITE, pages);
+			requested, FOLL_WRITE, pages + *nr_pages);
 		if (pinned < 0)
 			return pinned;
 
-		nr_pages -= pinned;
-		pages += pinned;
+		free -= pinned;
+		*nr_pages += pinned;
 	}
 
 	return 0;
 }
 
-static void unlock_pages(struct page *pages[], unsigned int nr_pages)
-{
-	unsigned int i;
-
-	if (!pages)
-		return;
-
-	for (i = 0; i < nr_pages; i++) {
-		if (pages[i])
-			put_page(pages[i]);
-	}
-}
-
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 {
 	struct privcmd_data *data = file->private_data;
@@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 
 	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
 	if (!xbufs) {
+		nr_pages = 0;
 		rc = -ENOMEM;
 		goto out;
 	}
 
-	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
+	rc = lock_pages(kbufs, kdata.num, pages, _pages);
 	if (rc)
 		goto out;
 
@@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 	xen_preemptible_hcall_end();
 
 out:
-	unlock_pages(pages, nr_pages);
+	if (pages)
+		put_user_pages(pages, nr_pages);
 	kfree(xbufs);
 	kfree(pages);
 	kfree(kbufs);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 20/34] xen: convert put_page() to put_user_page*()

[Juergen Gross]

On 02.08.19 04:19, john.hubb...@gmail.com wrote:

From: John Hubbard 

For pages that were retained via get_user_pages*(), release those pages
via the new put_user_page*() routines, instead of via put_page() or
release_pages().

This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
("mm: introduce put_user_page*(), placeholder versions").

Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: xen-de...@lists.xenproject.org
Signed-off-by: John Hubbard 
---
  drivers/xen/gntdev.c  | 5 +
  drivers/xen/privcmd.c | 7 +--
  2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 4c339c7e66e5..2586b3df2bb6 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -864,10 +864,7 @@ static int gntdev_get_page(struct gntdev_copy_batch 
*batch, void __user *virt,
  
  static void gntdev_put_pages(struct gntdev_copy_batch *batch)

  {
-   unsigned int i;
-
-   for (i = 0; i < batch->nr_pages; i++)
-   put_page(batch->pages[i]);
+   put_user_pages(batch->pages, batch->nr_pages);
batch->nr_pages = 0;
  }
  
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c

index 2f5ce7230a43..29e461dbee2d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,15 +611,10 @@ static int lock_pages(
  
  static void unlock_pages(struct page *pages[], unsigned int nr_pages)

  {
-   unsigned int i;
-
if (!pages)
return;
  
-	for (i = 0; i < nr_pages; i++) {

-   if (pages[i])
-   put_page(pages[i]);
-   }
+   put_user_pages(pages, nr_pages);


You are not handling the case where pages[i] is NULL here. Or am I
missing a pending patch to put_user_pages() here?


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] MAINTAINERS: unify reference to xen-devel list

[Juergen Gross]

On 12/01/2019 10:07, Lukas Bulwahn wrote:
> In the linux kernel MAINTAINERS file, largely
>   "xen-de...@lists.xenproject.org (moderated for non-subscribers)"
> is used to refer to the xen-devel mailing list.
> 
> The DRM DRIVERS FOR XEN section entry mentions
> xen-de...@lists.xen.org instead, but that is just the same
> mailing list as the mailing list above.
> 
> Signed-off-by: Lukas Bulwahn 

Reviewed-by: Juergen Gross 


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel][PATCH 1/3] xen: Introduce shared buffer helpers for page directory...

[Juergen Gross]

On 29/11/2018 12:22, Oleksandr Andrushchenko wrote:
> ping
> 
> On 11/22/18 12:02 PM, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>>
>> based frontends. Currently the frontends which implement
>> similar code for sharing big buffers between frontend and
>> backend are para-virtualized DRM and sound drivers.
>> Both define the same way to share grant references of a
>> data buffer with the corresponding backend with little
>> differences.
>>
>> Move shared code into a helper module, so there is a single
>> implementation of the same functionality for all.
>>
>> Signed-off-by: Oleksandr Andrushchenko 

In general I'm fine with this approach.

With the concerns raised for one of the other patches I wanted to wait
for V2 of the series. Or won't the resulting change require a
modification of this patch?

It would be nice if you could point out in the commit message whether
you are doing code movement (with some renames) only, or if there are
any functional changes involved (and which ones). This would make the
review much easier and less time consuming.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel][PATCH 2/3] drm/xen-front: Use Xen common shared buffer implementation

[Juergen Gross]

On 22/11/2018 15:33, Daniel Vetter wrote:
> On Thu, Nov 22, 2018 at 12:02:29PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>>
>> Use page directory based shared buffer implementation
>> now available as common code for Xen frontend drivers.
>>
>> Signed-off-by: Oleksandr Andrushchenko 
>> ---
>>  drivers/gpu/drm/xen/Kconfig   |   1 +
>>  drivers/gpu/drm/xen/Makefile  |   1 -
>>  drivers/gpu/drm/xen/xen_drm_front.c   |  60 ++--
>>  drivers/gpu/drm/xen/xen_drm_front_gem.c   |   1 -
>>  drivers/gpu/drm/xen/xen_drm_front_shbuf.c | 414 --
>>  drivers/gpu/drm/xen/xen_drm_front_shbuf.h |  64 
>>  6 files changed, 30 insertions(+), 511 deletions(-)
>>  delete mode 100644 drivers/gpu/drm/xen/xen_drm_front_shbuf.c
>>  delete mode 100644 drivers/gpu/drm/xen/xen_drm_front_shbuf.h
> Anyway, I'm all for code duplication removal, so if the Xen folks are
> happy with patch 1, this one here has my ack. Might also be best to merge
> all three through the Xen tree. Fallback would be xen folks sending a
> topic pull request with these 3 patches to drm-misc and takashi's sound
> tree.

I'm fine with taking it through the Xen tree.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 0/9] xen: dma-buf support for grant device

[Juergen Gross]

On 02/07/18 09:10, Oleksandr Andrushchenko wrote:
> Hello, Boris, Juergen!
> 
> Do you think I can re-base the series (which already has
> all required R-b's from Xen community) onto the latest kernel
> with API changes to patches 5 (of_dma_configure) and 8
> (dma-buf atomic ops) and we can merge it to the Xen's kernel tree?

Rebase: yes.

Merging to the Xen kernel tree: only after setting up the
for-linus-4.19 branch, which will be done by Boris later this
month.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/9] xen/gntdev: Allow mappings for DMA buffers

[Juergen Gross]

On 15/06/18 08:32, Oleksandr Andrushchenko wrote:
> Please note, that this will need a change (attached) while
> applying to the mainline kernel because of API changes [1].
> 
> Unfortunately, current Xen tip kernel tree is v4.17-rc5 based,
> so I cannot make the change in this patch now.

I don't see any chance this series could go into 4.18, as the merge
window is just closing. So please post the patch based on current
Linux master of torvalds/linux.git


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] xen/balloon: Move common memory reservation routines to a module

[Juergen Gross]

On 25/05/18 17:33, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
> 
> Memory {increase|decrease}_reservation and VA mappings update/reset
> code used in balloon driver can be made common, so other drivers can
> also re-use the same functionality without open-coding.
> Create a dedicated module for the shared code and export corresponding
> symbols for other kernel modules.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/Makefile  |   1 +
>  drivers/xen/balloon.c |  71 ++
>  drivers/xen/mem-reservation.c | 134 ++
>  include/xen/mem_reservation.h |  29 
>  4 files changed, 170 insertions(+), 65 deletions(-)
>  create mode 100644 drivers/xen/mem-reservation.c
>  create mode 100644 include/xen/mem_reservation.h

Can you please name this include/xen/mem-reservation.h ?

> 
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 451e833f5931..3c87b0c3aca6 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_HOTPLUG_CPU)+= cpu_hotplug.o
>  obj-$(CONFIG_X86)+= fallback.o
>  obj-y+= grant-table.o features.o balloon.o manage.o preempt.o time.o
> +obj-y+= mem-reservation.o
>  obj-y+= events/
>  obj-y+= xenbus/
>  
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 065f0b607373..57b482d67a3a 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -71,6 +71,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  static int xen_hotplug_unpopulated;
>  
> @@ -157,13 +158,6 @@ static DECLARE_DELAYED_WORK(balloon_worker, 
> balloon_process);
>  #define GFP_BALLOON \
>   (GFP_HIGHUSER | __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC)
>  
> -static void scrub_page(struct page *page)
> -{
> -#ifdef CONFIG_XEN_SCRUB_PAGES
> - clear_highpage(page);
> -#endif
> -}
> -
>  /* balloon_append: add the given page to the balloon. */
>  static void __balloon_append(struct page *page)
>  {
> @@ -463,11 +457,6 @@ static enum bp_state increase_reservation(unsigned long 
> nr_pages)
>   int rc;
>   unsigned long i;
>   struct page   *page;
> - struct xen_memory_reservation reservation = {
> - .address_bits = 0,
> - .extent_order = EXTENT_ORDER,
> - .domid= DOMID_SELF
> - };
>  
>   if (nr_pages > ARRAY_SIZE(frame_list))
>   nr_pages = ARRAY_SIZE(frame_list);
> @@ -486,9 +475,7 @@ static enum bp_state increase_reservation(unsigned long 
> nr_pages)
>   page = balloon_next_page(page);
>   }
>  
> - set_xen_guest_handle(reservation.extent_start, frame_list);
> - reservation.nr_extents = nr_pages;
> - rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, );
> + rc = xenmem_reservation_increase(nr_pages, frame_list);
>   if (rc <= 0)
>   return BP_EAGAIN;
>  
> @@ -496,29 +483,7 @@ static enum bp_state increase_reservation(unsigned long 
> nr_pages)
>   page = balloon_retrieve(false);
>   BUG_ON(page == NULL);
>  
> -#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);
> -
> - if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> - unsigned long pfn = page_to_pfn(page);
> -
> - set_phys_to_machine(pfn, frame_list[i]);
> -
> - /* Link back into the page tables if not highmem. */
> - if (!PageHighMem(page)) {
> - int ret;
> - ret = HYPERVISOR_update_va_mapping(
> - (unsigned long)__va(pfn << 
> PAGE_SHIFT),
> - mfn_pte(frame_list[i], 
> PAGE_KERNEL),
> - 0);
> - BUG_ON(ret);
> - }
> - }
> -#endif
> + xenmem_reservation_va_mapping_update(1, , _list[i]);
>  
>   /* Relinquish the page back to the allocator. */
>   free_reserved_page(page);
> @@ -535,11 +500,6 @@ static enum bp_state decrease_reservation(unsigned long 
> nr_pages, gfp_t gfp)
>   unsigned long i;
>   struct page *page, *tmp;
>   int ret;
> - struct xen_memory_reservation reservation = {
> - .address_bits = 0,
> - .extent_order = EXTENT_ORDER,
> - .domid= DOMID_SELF
> - };
>   LIST_HEAD(pages);
>  
>   if (nr_pages > ARRAY_SIZE(frame_list))
> @@ -553,7 +513,7 @@ static enum bp_state decrease_reservation(unsigned long 
> nr_pages, gfp_t gfp)
>   break;
>   }
>   adjust_managed_page_count(page, 

Re: [PATCH 1/8] xen/grant-table: Make set/clear page private code shared

[Juergen Gross]

On 25/05/18 17:33, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
> 
> Make set/clear page private code shared and accessible to
> other kernel modules which can re-use these instead of open-coding.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/grant-table.c | 54 +--
>  include/xen/grant_table.h |  3 +++
>  2 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 27be107d6480..d7488226e1f2 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -769,29 +769,18 @@ void gnttab_free_auto_xlat_frames(void)
>  }
>  EXPORT_SYMBOL_GPL(gnttab_free_auto_xlat_frames);
>  
> -/**
> - * gnttab_alloc_pages - alloc pages suitable for grant mapping into
> - * @nr_pages: number of pages to alloc
> - * @pages: returns the pages
> - */
> -int gnttab_alloc_pages(int nr_pages, struct page **pages)
> +int gnttab_pages_set_private(int nr_pages, struct page **pages)
>  {
>   int i;
> - int ret;
> -
> - ret = alloc_xenballooned_pages(nr_pages, pages);
> - if (ret < 0)
> - return ret;
>  
>   for (i = 0; i < nr_pages; i++) {
>  #if BITS_PER_LONG < 64
>   struct xen_page_foreign *foreign;
>  
>   foreign = kzalloc(sizeof(*foreign), GFP_KERNEL);
> - if (!foreign) {
> - gnttab_free_pages(nr_pages, pages);
> + if (!foreign)
>   return -ENOMEM;
> - }
> +
>   set_page_private(pages[i], (unsigned long)foreign);
>  #endif
>   SetPagePrivate(pages[i]);
> @@ -799,14 +788,30 @@ int gnttab_alloc_pages(int nr_pages, struct page 
> **pages)
>  
>   return 0;
>  }
> -EXPORT_SYMBOL(gnttab_alloc_pages);
> +EXPORT_SYMBOL(gnttab_pages_set_private);

EXPORT_SYMBOL_GPL()

>  
>  /**
> - * gnttab_free_pages - free pages allocated by gnttab_alloc_pages()
> - * @nr_pages; number of pages to free
> - * @pages: the pages
> + * gnttab_alloc_pages - alloc pages suitable for grant mapping into
> + * @nr_pages: number of pages to alloc
> + * @pages: returns the pages
>   */
> -void gnttab_free_pages(int nr_pages, struct page **pages)
> +int gnttab_alloc_pages(int nr_pages, struct page **pages)
> +{
> + int ret;
> +
> + ret = alloc_xenballooned_pages(nr_pages, pages);
> + if (ret < 0)
> + return ret;
> +
> + ret = gnttab_pages_set_private(nr_pages, pages);
> + if (ret < 0)
> + gnttab_free_pages(nr_pages, pages);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(gnttab_alloc_pages);
> +
> +void gnttab_pages_clear_private(int nr_pages, struct page **pages)
>  {
>   int i;
>  
> @@ -818,6 +823,17 @@ void gnttab_free_pages(int nr_pages, struct page **pages)
>   ClearPagePrivate(pages[i]);
>   }
>   }
> +}
> +EXPORT_SYMBOL(gnttab_pages_clear_private);

EXPORT_SYMBOL_GPL()


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/xen-front: fix pointer casts

[Juergen Gross]

On 23/05/18 13:36, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
> 
> Building for a 32-bit target results in warnings from casting
> between a 32-bit pointer and a 64-bit integer. Fix the warnings
> by casting those pointers to uintptr_t first.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/xen-front: fix pointer casts

[Juergen Gross]

On 21/05/18 09:39, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
> 
> Building for a 32-bit target results in warnings from casting
> between a 32-bit pointer and a 64-bit integer. Fix the warnings
> by casting those pointers to uintptr_t first.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/gpu/drm/xen/xen_drm_front.h   | 4 ++--
>  drivers/gpu/drm/xen/xen_drm_front_shbuf.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.h 
> b/drivers/gpu/drm/xen/xen_drm_front.h
> index 2c2479b571ae..8e15dbebc4ba 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.h
> +++ b/drivers/gpu/drm/xen/xen_drm_front.h
> @@ -126,12 +126,12 @@ struct xen_drm_front_drm_info {
>  
>  static inline u64 xen_drm_front_fb_to_cookie(struct drm_framebuffer *fb)
>  {
> - return (u64)fb;
> + return (u64)(uintptr_t)fb;

Do you really still need the cast to u64?

>  }
>  
>  static inline u64 xen_drm_front_dbuf_to_cookie(struct drm_gem_object 
> *gem_obj)
>  {
> - return (u64)gem_obj;
> + return (u64)(uintptr_t)gem_obj;
>  }
>  
>  int xen_drm_front_mode_set(struct xen_drm_front_drm_pipeline *pipeline,
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_shbuf.c 
> b/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
> index 8099cb343ae3..47fc93847765 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
> @@ -122,7 +122,7 @@ static void guest_calc_num_grefs(struct 
> xen_drm_front_shbuf *buf)
>  }
>  
>  #define xen_page_to_vaddr(page) \
> - ((phys_addr_t)pfn_to_kaddr(page_to_xen_pfn(page)))
> + ((phys_addr_t)(uintptr_t)pfn_to_kaddr(page_to_xen_pfn(page)))

phys_addr_t for a virtual address?


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/xen-front: fix pointer casts

[Juergen Gross]

On 23/05/18 12:00, Oleksandr Andrushchenko wrote:
> On 05/23/2018 12:19 PM, Juergen Gross wrote:
>> On 21/05/18 09:39, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>
>>> Building for a 32-bit target results in warnings from casting
>>> between a 32-bit pointer and a 64-bit integer. Fix the warnings
>>> by casting those pointers to uintptr_t first.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <oleksandr_andrushche...@epam.com>
>>> ---
>>>   drivers/gpu/drm/xen/xen_drm_front.h   | 4 ++--
>>>   drivers/gpu/drm/xen/xen_drm_front_shbuf.c | 2 +-
>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front.h
>>> b/drivers/gpu/drm/xen/xen_drm_front.h
>>> index 2c2479b571ae..8e15dbebc4ba 100644
>>> --- a/drivers/gpu/drm/xen/xen_drm_front.h
>>> +++ b/drivers/gpu/drm/xen/xen_drm_front.h
>>> @@ -126,12 +126,12 @@ struct xen_drm_front_drm_info {
>>>     static inline u64 xen_drm_front_fb_to_cookie(struct
>>> drm_framebuffer *fb)
>>>   {
>>> -    return (u64)fb;
>>> +    return (u64)(uintptr_t)fb;
>> Do you really still need the cast to u64?
> Indeed, I can remove (u64) now, thank you
>>
>>>   }
>>>     static inline u64 xen_drm_front_dbuf_to_cookie(struct
>>> drm_gem_object *gem_obj)
>>>   {
>>> -    return (u64)gem_obj;
>>> +    return (u64)(uintptr_t)gem_obj;
>>>   }
>>>     int xen_drm_front_mode_set(struct xen_drm_front_drm_pipeline
>>> *pipeline,
>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
>>> b/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
>>> index 8099cb343ae3..47fc93847765 100644
>>> --- a/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_shbuf.c
>>> @@ -122,7 +122,7 @@ static void guest_calc_num_grefs(struct
>>> xen_drm_front_shbuf *buf)
>>>   }
>>>     #define xen_page_to_vaddr(page) \
>>> -    ((phys_addr_t)pfn_to_kaddr(page_to_xen_pfn(page)))
>>> +    ((phys_addr_t)(uintptr_t)pfn_to_kaddr(page_to_xen_pfn(page)))
>> phys_addr_t for a virtual address?
> This is because the resulting value is then passed to gnttab_set_map_op/
> gnttab_set_unmap_op which expects host address to be passed as
> phys_addr_t addr :(

Okay, but this means the compiler should do the necessary cast when
you drop the cast to phys_addr_t in xen_page_to_vaddr(), as the cast
to uintptr_t is already producing an unsigned integer value which can
easily be promoted to more bits, right?


Juergen
___
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

[Juergen Gross]

On 24/04/18 12:14, Oleksandr Andrushchenko wrote:
> On 04/24/2018 01:01 PM, Wei Liu wrote:
>> On Tue, Apr 24, 2018 at 11:08:41AM +0200, Juergen Gross wrote:
>>> On 24/04/18 11:03, Oleksandr Andrushchenko wrote:
>>>> On 04/24/2018 11:40 AM, Juergen Gross wrote:
>>>>> On 24/04/18 10:07, Oleksandr Andrushchenko wrote:
>>>>>> On 04/24/2018 10:51 AM, Juergen Gross wrote:
>>>>>>> On 24/04/18 07:43, Oleksandr Andrushchenko wrote:
>>>>>>>> On 04/24/2018 01:41 AM, Boris Ostrovsky wrote:
>>>>>>>>> On 04/23/2018 08:10 AM, Oleksandr Andrushchenko wrote:
>>>>>>>>>> On 04/23/2018 02:52 PM, Wei Liu wrote:
>>>>>>>>>>> On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr
>>>>>>>>>>> Andrushchenko
>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>  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.
>>>>>>>>>>>>> You can't just wrap random userspace memory into a dma-buf.
>>>>>>>>>>>>> We've
>>>>>>>>>>>>> just had
>>>>>>>>>>>>> this discussion with kvm/qemu folks, who proposed just
>>>>>>>>>>>>> that, and
>>>>>>>>>>>>> after a
>>>>>>>>>>>>> bit of discussion they'll now try to have a driver which just
>>>>>>>>>>>>> wraps a
>>>>>>>>>>>>> memfd into a dma-buf.
>>>>>>>>>>>> So, we have to decide either we introduce a new driver
>>>>>>>>>>>> (say, under drivers/xen/xen-dma-buf) or extend the existing
>>>>>>>>>>>> gntdev/balloon to support dma-buf use-cases.
>>>>>>>>>>>>
>>>>>>>>>>>> Can anybody from Xen community express their preference here?
>>>>>>>>>>>>
>>>>>>>>>>> Oleksandr talked to me on IRC about this, he said a few IOCTLs
>>>>>>>>>>> need to
>>>>>>>>>>> be added to either existing drivers or a new driver.
>>>>>>>>>>>
>>>>>>>>>>> I went through this thread twice and skimmed through the
>>>>>>>>>>> relevant
>>>>>>>>>>> documents, but I couldn't see any obvious pros and cons for
>>>>>>>>>>> either
>>>>>>>>>>> approach. So I don't really have an opinion on this.
>>>>>>>>>>>
>>>>>>>>>>> But, assuming if implemented in existing drivers, those IOCTLs
>>>>>>>>>>> need to
>>>>>>>>>>> be added to different drivers, which means userspace program
>>>>>>>>>>> needs to
>>>>>>>>>>> write more code and get more handles, it would be slightly
>>>>>>>>>>> better to
>>>>>>>>>>> implement a new driver from that perspective.
>>>>>>>>>> If gntdev/balloon extension is still considered:
>>>>>>>>>>
>>>>>>>>>> All the IOCTLs will be in gntdev driver (in current xen-zcopy
>>>>>>>>>> terminology):
>>>>>>>>>>  - DRM_ICOTL_XEN_ZCOPY_DUMB_FROM_REFS
>>>>>>>>>>  - DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
>>>>>>>>>>  - DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE
>>>>>>>>>>
>>>>>>>>>> Balloon driver extension, which is needed for contiguous/DMA
>>>>>>>>>> buffers, will be to provide new *kernel API*, no UAPI is needed.
>>>>>>>>>>
>>>>>>>>> So I am obviously a bit late to this thread, but why do you need
>>>&g

Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

[Juergen Gross]

On 24/04/18 07:43, Oleksandr Andrushchenko wrote:
> On 04/24/2018 01:41 AM, Boris Ostrovsky wrote:
>> On 04/23/2018 08:10 AM, Oleksandr Andrushchenko wrote:
>>> On 04/23/2018 02:52 PM, Wei Liu wrote:
 On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr Andrushchenko
 wrote:
>>>   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.
>> You can't just wrap random userspace memory into a dma-buf. We've
>> just had
>> this discussion with kvm/qemu folks, who proposed just that, and
>> after a
>> bit of discussion they'll now try to have a driver which just wraps a
>> memfd into a dma-buf.
> So, we have to decide either we introduce a new driver
> (say, under drivers/xen/xen-dma-buf) or extend the existing
> gntdev/balloon to support dma-buf use-cases.
>
> Can anybody from Xen community express their preference here?
>
 Oleksandr talked to me on IRC about this, he said a few IOCTLs need to
 be added to either existing drivers or a new driver.

 I went through this thread twice and skimmed through the relevant
 documents, but I couldn't see any obvious pros and cons for either
 approach. So I don't really have an opinion on this.

 But, assuming if implemented in existing drivers, those IOCTLs need to
 be added to different drivers, which means userspace program needs to
 write more code and get more handles, it would be slightly better to
 implement a new driver from that perspective.
>>> If gntdev/balloon extension is still considered:
>>>
>>> All the IOCTLs will be in gntdev driver (in current xen-zcopy
>>> terminology):
>>>   - DRM_ICOTL_XEN_ZCOPY_DUMB_FROM_REFS
>>>   - DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
>>>   - DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE
>>>
>>> Balloon driver extension, which is needed for contiguous/DMA
>>> buffers, will be to provide new *kernel API*, no UAPI is needed.
>>>
>>
>> So I am obviously a bit late to this thread, but why do you need to add
>> new ioctls to gntdev and balloon? Doesn't this driver manage to do what
>> you want without any extensions?
> 1. I only (may) need to add IOCTLs to gntdev
> 2. balloon driver needs to be extended, so it can allocate
> contiguous (DMA) memory, not IOCTLs/UAPI here, all lives
> in the kernel.
> 3. The reason I need to extend gnttab with new IOCTLs is to
> provide new functionality to create a dma-buf from grant references
> and to produce grant references for a dma-buf. This is what I have as UAPI
> description for xen-zcopy driver:
> 
> 1. DRM_IOCTL_XEN_ZCOPY_DUMB_FROM_REFS
> This will create a DRM dumb buffer from grant references provided
> by the frontend. The intended usage is:
>   - Frontend
>     - creates a dumb/display buffer and allocates memory
>     - grants foreign access to the buffer pages
>     - passes granted references to the backend
>   - Backend
>     - issues DRM_XEN_ZCOPY_DUMB_FROM_REFS ioctl to map
>   granted references and create a dumb buffer
>     - requests handle to fd conversion via DRM_IOCTL_PRIME_HANDLE_TO_FD
>     - requests real HW driver/consumer to import the PRIME buffer with
>   DRM_IOCTL_PRIME_FD_TO_HANDLE
>     - uses handle returned by the real HW driver
>   - at the end:
>     o closes real HW driver's handle with DRM_IOCTL_GEM_CLOSE
>     o closes zero-copy driver's handle with DRM_IOCTL_GEM_CLOSE
>     o closes file descriptor of the exported buffer
> 
> 2. DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
> This will grant references to a dumb/display buffer's memory provided by
> the
> backend. The intended usage is:
>   - Frontend
>     - requests backend to allocate dumb/display buffer and grant references
>   to its pages
>   - Backend
>     - requests real HW driver to create a dumb with
> DRM_IOCTL_MODE_CREATE_DUMB
>     - requests handle to fd conversion via DRM_IOCTL_PRIME_HANDLE_TO_FD
>     - requests zero-copy driver to import the PRIME buffer with
>   DRM_IOCTL_PRIME_FD_TO_HANDLE
>     - issues DRM_XEN_ZCOPY_DUMB_TO_REFS ioctl to
>   grant references to the buffer's memory.
>     - passes grant references to the frontend
>  - at the end:
>     - closes zero-copy driver's handle with DRM_IOCTL_GEM_CLOSE
>     - closes real HW driver's handle with DRM_IOCTL_GEM_CLOSE
>     - closes file descriptor of the imported buffer
> 
> 3. DRM_XEN_ZCOPY_DUMB_WAIT_FREE
> This will block until the dumb buffer with the wait handle provided be
> freed:
> this is needed for synchronization between frontend and backend in case
> frontend provides grant references of the buffer via
> DRM_XEN_ZCOPY_DUMB_FROM_REFS IOCTL and which must be released before
> backend replies with XENDISPL_OP_DBUF_DESTROY response.
> wait_handle must be the same value returned while calling
> DRM_XEN_ZCOPY_DUMB_FROM_REFS IOCTL.
> 
> So, as you can see the above functionality is not 

Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

[Juergen Gross]

On 24/04/18 10:07, Oleksandr Andrushchenko wrote:
> On 04/24/2018 10:51 AM, Juergen Gross wrote:
>> On 24/04/18 07:43, Oleksandr Andrushchenko wrote:
>>> On 04/24/2018 01:41 AM, Boris Ostrovsky wrote:
>>>> On 04/23/2018 08:10 AM, Oleksandr Andrushchenko wrote:
>>>>> On 04/23/2018 02:52 PM, Wei Liu wrote:
>>>>>> On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr Andrushchenko
>>>>>> wrote:
>>>>>>>>>    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.
>>>>>>>> You can't just wrap random userspace memory into a dma-buf. We've
>>>>>>>> just had
>>>>>>>> this discussion with kvm/qemu folks, who proposed just that, and
>>>>>>>> after a
>>>>>>>> bit of discussion they'll now try to have a driver which just
>>>>>>>> wraps a
>>>>>>>> memfd into a dma-buf.
>>>>>>> So, we have to decide either we introduce a new driver
>>>>>>> (say, under drivers/xen/xen-dma-buf) or extend the existing
>>>>>>> gntdev/balloon to support dma-buf use-cases.
>>>>>>>
>>>>>>> Can anybody from Xen community express their preference here?
>>>>>>>
>>>>>> Oleksandr talked to me on IRC about this, he said a few IOCTLs
>>>>>> need to
>>>>>> be added to either existing drivers or a new driver.
>>>>>>
>>>>>> I went through this thread twice and skimmed through the relevant
>>>>>> documents, but I couldn't see any obvious pros and cons for either
>>>>>> approach. So I don't really have an opinion on this.
>>>>>>
>>>>>> But, assuming if implemented in existing drivers, those IOCTLs
>>>>>> need to
>>>>>> be added to different drivers, which means userspace program needs to
>>>>>> write more code and get more handles, it would be slightly better to
>>>>>> implement a new driver from that perspective.
>>>>> If gntdev/balloon extension is still considered:
>>>>>
>>>>> All the IOCTLs will be in gntdev driver (in current xen-zcopy
>>>>> terminology):
>>>>>    - DRM_ICOTL_XEN_ZCOPY_DUMB_FROM_REFS
>>>>>    - DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
>>>>>    - DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE
>>>>>
>>>>> Balloon driver extension, which is needed for contiguous/DMA
>>>>> buffers, will be to provide new *kernel API*, no UAPI is needed.
>>>>>
>>>> So I am obviously a bit late to this thread, but why do you need to add
>>>> new ioctls to gntdev and balloon? Doesn't this driver manage to do what
>>>> you want without any extensions?
>>> 1. I only (may) need to add IOCTLs to gntdev
>>> 2. balloon driver needs to be extended, so it can allocate
>>> contiguous (DMA) memory, not IOCTLs/UAPI here, all lives
>>> in the kernel.
>>> 3. The reason I need to extend gnttab with new IOCTLs is to
>>> provide new functionality to create a dma-buf from grant references
>>> and to produce grant references for a dma-buf. This is what I have as
>>> UAPI
>>> description for xen-zcopy driver:
>>>
>>> 1. DRM_IOCTL_XEN_ZCOPY_DUMB_FROM_REFS
>>> This will create a DRM dumb buffer from grant references provided
>>> by the frontend. The intended usage is:
>>>    - Frontend
>>>  - creates a dumb/display buffer and allocates memory
>>>  - grants foreign access to the buffer pages
>>>  - passes granted references to the backend
>>>    - Backend
>>>  - issues DRM_XEN_ZCOPY_DUMB_FROM_REFS ioctl to map
>>>    granted references and create a dumb buffer
>>>  - requests handle to fd conversion via DRM_IOCTL_PRIME_HANDLE_TO_FD
>>>  - requests real HW driver/consumer to import the PRIME buffer with
>>>    DRM_IOCTL_PRIME_FD_TO_HANDLE
>>>  - uses handle returned by the real HW driver
>>>    - at the end:
>>>  o closes real HW driver's handle with DRM_IOCTL_GEM_CLOSE
>>>  o closes zero-copy driver's handle with DRM_IOCTL_GEM_CLOSE
>

Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

[Juergen Gross]

On 24/04/18 11:03, Oleksandr Andrushchenko wrote:
> On 04/24/2018 11:40 AM, Juergen Gross wrote:
>> On 24/04/18 10:07, Oleksandr Andrushchenko wrote:
>>> On 04/24/2018 10:51 AM, Juergen Gross wrote:
>>>> On 24/04/18 07:43, Oleksandr Andrushchenko wrote:
>>>>> On 04/24/2018 01:41 AM, Boris Ostrovsky wrote:
>>>>>> On 04/23/2018 08:10 AM, Oleksandr Andrushchenko wrote:
>>>>>>> On 04/23/2018 02:52 PM, Wei Liu wrote:
>>>>>>>> On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr Andrushchenko
>>>>>>>> wrote:
>>>>>>>>>>>     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.
>>>>>>>>>> You can't just wrap random userspace memory into a dma-buf. We've
>>>>>>>>>> just had
>>>>>>>>>> this discussion with kvm/qemu folks, who proposed just that, and
>>>>>>>>>> after a
>>>>>>>>>> bit of discussion they'll now try to have a driver which just
>>>>>>>>>> wraps a
>>>>>>>>>> memfd into a dma-buf.
>>>>>>>>> So, we have to decide either we introduce a new driver
>>>>>>>>> (say, under drivers/xen/xen-dma-buf) or extend the existing
>>>>>>>>> gntdev/balloon to support dma-buf use-cases.
>>>>>>>>>
>>>>>>>>> Can anybody from Xen community express their preference here?
>>>>>>>>>
>>>>>>>> Oleksandr talked to me on IRC about this, he said a few IOCTLs
>>>>>>>> need to
>>>>>>>> be added to either existing drivers or a new driver.
>>>>>>>>
>>>>>>>> I went through this thread twice and skimmed through the relevant
>>>>>>>> documents, but I couldn't see any obvious pros and cons for either
>>>>>>>> approach. So I don't really have an opinion on this.
>>>>>>>>
>>>>>>>> But, assuming if implemented in existing drivers, those IOCTLs
>>>>>>>> need to
>>>>>>>> be added to different drivers, which means userspace program
>>>>>>>> needs to
>>>>>>>> write more code and get more handles, it would be slightly
>>>>>>>> better to
>>>>>>>> implement a new driver from that perspective.
>>>>>>> If gntdev/balloon extension is still considered:
>>>>>>>
>>>>>>> All the IOCTLs will be in gntdev driver (in current xen-zcopy
>>>>>>> terminology):
>>>>>>> - DRM_ICOTL_XEN_ZCOPY_DUMB_FROM_REFS
>>>>>>> - DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
>>>>>>> - DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE
>>>>>>>
>>>>>>> Balloon driver extension, which is needed for contiguous/DMA
>>>>>>> buffers, will be to provide new *kernel API*, no UAPI is needed.
>>>>>>>
>>>>>> So I am obviously a bit late to this thread, but why do you need
>>>>>> to add
>>>>>> new ioctls to gntdev and balloon? Doesn't this driver manage to do
>>>>>> what
>>>>>> you want without any extensions?
>>>>> 1. I only (may) need to add IOCTLs to gntdev
>>>>> 2. balloon driver needs to be extended, so it can allocate
>>>>> contiguous (DMA) memory, not IOCTLs/UAPI here, all lives
>>>>> in the kernel.
>>>>> 3. The reason I need to extend gnttab with new IOCTLs is to
>>>>> provide new functionality to create a dma-buf from grant references
>>>>> and to produce grant references for a dma-buf. This is what I have as
>>>>> UAPI
>>>>> description for xen-zcopy driver:
>>>>>
>>>>> 1. DRM_IOCTL_XEN_ZCOPY_DUMB_FROM_REFS
>>>>> This will create a DRM dumb buffer from grant references provided
>>>>> by the frontend. The intended usage is:
>>>>>     - Frontend
>>>>>   - creates a dumb/display 

Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

[Juergen Gross]

On 24/04/18 22:35, Dongwon Kim wrote:
> Had a meeting with Daniel and talked about bringing out generic
> part of hyper-dmabuf to the userspace, which means we most likely
> reuse IOCTLs defined in xen-zcopy for our use-case if we follow
> his suggestion.
> 
> So assuming we use these IOCTLs as they are,
> Several things I would like you to double-check..
> 
> 1. returning gref as is to the user space is still unsafe because
> it is a constant, easy to guess and any process that hijacks it can easily
> exploit the buffer. So I am wondering if it's possible to keep dmabuf-to
> -gref or gref-to-dmabuf in kernel space and add other layers on top
> of those in actual IOCTLs to add some safety.. We introduced flink like
> hyper_dmabuf_id including random number but many says even that is still
> not safe.

grefs are usable by root only. When you have root access in dom0 you can
do evil things to all VMs even without using grants. That is in no way
different to root being able to control all other processes on the
system.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/xen-front: Implement Xen bus state handling

[Juergen Gross]

On 21/02/18 09:03, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
> 
> Initial handling for Xen bus states: implement
> Xen bus state machine for the frontend driver according to
> the state diagram and recovery flow from display para-virtualized
> protocol: xen/interface/io/displif.h.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/gpu/drm/xen/xen_drm_front.c | 124 
> +++-
>  drivers/gpu/drm/xen/xen_drm_front.h |  26 
>  2 files changed, 149 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/xen/xen_drm_front.h
> 
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
> b/drivers/gpu/drm/xen/xen_drm_front.c
> index fd372fb464a1..d0306f9d660d 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
> @@ -24,19 +24,141 @@
>  
>  #include 
>  
> +#include "xen_drm_front.h"
> +
> +static void xen_drv_remove_internal(struct xen_drm_front_info *front_info)
> +{
> +}
> +
> +static int backend_on_initwait(struct xen_drm_front_info *front_info)
> +{
> + return 0;
> +}
> +
> +static int backend_on_connected(struct xen_drm_front_info *front_info)
> +{
> + return 0;
> +}
> +
> +static void backend_on_disconnected(struct xen_drm_front_info *front_info)
> +{
> + xenbus_switch_state(front_info->xb_dev, XenbusStateInitialising);
> +}
> +
>  static void backend_on_changed(struct xenbus_device *xb_dev,
>   enum xenbus_state backend_state)
>  {
> + struct xen_drm_front_info *front_info = dev_get_drvdata(_dev->dev);
> + int ret;
> +
> + DRM_DEBUG("Backend state is %s, front is %s\n",
> + xenbus_strstate(backend_state),
> + xenbus_strstate(xb_dev->state));
> +
> + switch (backend_state) {
> + case XenbusStateReconfiguring:
> + /* fall through */
> + case XenbusStateReconfigured:
> + /* fall through */
> + case XenbusStateInitialised:
> + break;
> +
> + case XenbusStateInitialising:
> + /* recovering after backend unexpected closure */
> + backend_on_disconnected(front_info);
> + break;
> +
> + case XenbusStateInitWait:
> + /* recovering after backend unexpected closure */
> + backend_on_disconnected(front_info);
> + if (xb_dev->state != XenbusStateInitialising)
> + break;
> +
> + ret = backend_on_initwait(front_info);
> + if (ret < 0)
> + xenbus_dev_fatal(xb_dev, ret, "initializing frontend");
> + else
> + xenbus_switch_state(xb_dev, XenbusStateInitialised);
> + break;
> +
> + case XenbusStateConnected:
> + if (xb_dev->state != XenbusStateInitialised)
> + break;
> +
> + ret = backend_on_connected(front_info);
> + if (ret < 0)
> + xenbus_dev_fatal(xb_dev, ret, "initializing DRM 
> driver");
> + else
> + xenbus_switch_state(xb_dev, XenbusStateConnected);
> + break;
> +
> + case XenbusStateClosing:
> + /*
> +  * in this state backend starts freeing resources,
> +  * so let it go into closed state, so we can also
> +  * remove ours
> +  */
> + break;
> +
> + case XenbusStateUnknown:
> + /* fall through */
> + case XenbusStateClosed:
> + if (xb_dev->state == XenbusStateClosed)
> + break;
> +
> + backend_on_disconnected(front_info);
> + break;
> + }
>  }
>  
>  static int xen_drv_probe(struct xenbus_device *xb_dev,
>   const struct xenbus_device_id *id)
>  {
> - return 0;
> + struct xen_drm_front_info *front_info;
> +
> + front_info = devm_kzalloc(_dev->dev,
> + sizeof(*front_info), GFP_KERNEL);
> + if (!front_info) {
> + xenbus_dev_fatal(xb_dev, -ENOMEM, "allocating device memory");

No need for message in case of allocation failure: this is
handled in memory allocation already.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/xen-front: Introduce Xen para-virtualized frontend driver

[Juergen Gross]

On 21/02/18 09:03, 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.

Use SPDX identifier instead (same applies for all other new
sources):

// SPDX-License-Identifier: GPL-2.0

> + *
> + * Copyright (C) 2016-2018 EPAM Systems Inc.
> + *
> + * Author: Oleksandr Andrushchenko 
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +static void backend_on_changed(struct xenbus_device *xb_dev,
> + enum xenbus_state backend_state)
> +{
> +}
> +
> +static int xen_drv_probe(struct xenbus_device *xb_dev,
> + const struct xenbus_device_id *id)
> +{
> + return 0;
> +}
> +
> +static int xen_drv_remove(struct xenbus_device *dev)
> +{
> + return 0;
> +}
> +
> +static const struct xenbus_device_id xen_drv_ids[] = {
> + { XENDISPL_DRIVER_NAME },
> + { "" }
> +};
> +
> +static struct xenbus_driver xen_driver = {
> + .ids = xen_drv_ids,
> + .probe = xen_drv_probe,
> + .remove = xen_drv_remove,
> + .otherend_changed = backend_on_changed,
> +};
> +
> +static int __init xen_drv_init(void)
> +{
> + if (!xen_domain())
> + return -ENODEV;
> +
> + if (xen_initial_domain()) {
> + DRM_ERROR(XENDISPL_DRIVER_NAME " cannot run in initial 
> domain\n");
> + return -ENODEV;
> + }

Why not? Wouldn't that be possible in case of the backend living in a
driver domain?


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org

Re: [PATCH 1/9] drm/xen-front: Introduce Xen para-virtualized frontend driver

[Juergen Gross]

On 21/02/18 09:47, Oleksandr Andrushchenko wrote:
> On 02/21/2018 10:19 AM, Juergen Gross wrote:
>> On 21/02/18 09:03, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>
>>> Introduce skeleton of the para-virtualized Xen display
>>> frontend driver. This patch only adds required
>>> essential stubs.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <oleksandr_andrushche...@epam.com>
>>> ---
>>>   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.
>> Use SPDX identifier instead (same applies for all other new
>> sources):
>>
>> // SPDX-License-Identifier: GPL-2.0
> Will update, thank you
>>> + *
>>> + * Copyright (C) 2016-2018 EPAM Systems Inc.
>>> + *
>>> + * Author: 

Re: [Xen-devel] [RFC PATCH 01/60] hyper_dmabuf: initial working version of hyper_dmabuf drv

[Juergen Gross]

On 20/12/17 00:27, Dongwon Kim wrote:
> I forgot to include this brief information about this patch series.
> 
> This patch series contains the implementation of a new device driver,
> hyper_dmabuf, which provides a method for DMA-BUF sharing across
> different OSes running on the same virtual OS platform powered by
> a hypervisor.

Some general remarks regarding this series:

You are starting the whole driver in drivers/xen/ and in the last patch
you move it over to drivers/dma-buf/. Why don't you use drivers/dma-buf/
from the beginning? The same applies to e.g. patch 22 changing the
license. Please make it easier for the reviewers by not letting us
review the development history of your work.

Please run ./scripts/checkpatch.pl on each patch and correct the issues
it is reporting. At the first glance I've seen several style problems
which I won't comment until the next round.

Please add the maintainers as Cc:, not only the related mailing lists.
As you seem to aim supporting other hypervisors than Xen you might want
to add virtualizat...@lists.linux-foundation.org as well.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] GPU hang with kernel 4.10rc3

[Juergen Gross]

On 11/05/17 23:08, Pavel Machek wrote:
> On Mon 2017-01-23 10:39:27, Juergen Gross wrote:
>> On 13/01/17 15:41, Juergen Gross wrote:
>>> On 12/01/17 10:21, Chris Wilson wrote:
>>>> On Thu, Jan 12, 2017 at 07:03:25AM +0100, Juergen Gross wrote:
>>>>> On 11/01/17 18:08, Chris Wilson wrote:
>>>>>> On Wed, Jan 11, 2017 at 05:33:34PM +0100, Juergen Gross wrote:
>>>>>>> With kernel 4.10rc3 running as Xen dm0 I get at each boot:
>>>>>>>
>>>>>>> [   49.213697] [drm] GPU HANG: ecode 7:0:0x3d1d3d3d, in gnome-shell
>>>>>>> [1431], reason: Hang on render ring, action: reset
>>>>>>> [   49.213699] [drm] GPU hangs can indicate a bug anywhere in the entire
>>>>>>> gfx stack, including userspace.
>>>>>>> [   49.213700] [drm] Please file a _new_ bug report on
>>>>>>> bugs.freedesktop.org against DRI -> DRM/Intel
>>>>>>> [   49.213700] [drm] drm/i915 developers can then reassign to the right
>>>>>>> component if it's not a kernel issue.
>>>>>>> [   49.213700] [drm] The gpu crash dump is required to analyze gpu
>>>>>>> hangs, so please always attach it.
>>>>>>> [   49.213701] [drm] GPU crash dump saved to /sys/class/drm/card0/error
>>>>>>> [   49.213755] drm/i915: Resetting chip after gpu hang
>>>>>>> [   60.213769] drm/i915: Resetting chip after gpu hang
>>>>>>> [   71.189737] drm/i915: Resetting chip after gpu hang
>>>>>>> [   82.165747] drm/i915: Resetting chip after gpu hang
>>>>>>> [   93.205727] drm/i915: Resetting chip after gpu hang
>>>>>>>
>>>>>>> The dump is attached.
>>>>>>
>>>>>> That's a nasty one. The first couple of pages of the batchbuffer appear
>>>>>> to be overwritten. (Full of 0xc2c2c2c2, i.e. probably pixel data.) That
>>>>>> may be a concurrent write by either the GPU or CPU, or we may have
>>>>>> incorrected mapped a set of pages. That it doesn't recovered suggests
>>>>>> that the corruption occurs frequently, probably on every request/batch.
>>>>>
>>>>> I hoped someone would have an idea already.
>>>>
>>>> Sorry, first report of something like this in a long time (that I can
>>>> remember at least). And the problem is that it can be anything from a
>>>> coherency to a concurrency issue, so no one patch springs to mind.
>>>> Thankfully it appears to be kernel related.
>>>> -Chris
>>>>
>>>
>>> Bisecting took longer than I thought, but I had to cherry pick some
>>> patches and rebase one of them multiple times...
>>>
>>> Finally I found the commit to blame: 920cf4194954ec ("drm/i915:
>>> Introduce an internal allocator for disposable private objects")
>>>
>>> In case you need me to produce some more data or test a patch
>>> feel free to reach out.
>>
>> Anything new for this severe regression?
>>
>> Without a fix 4.10 will be unusable with Xen on a machine with i915
>> graphics!
> 
> Did this get solved?

Yes. Commit 7152187159193056f30ad5726741bb25028672bf.


Juergen

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i915: fix i915 running as dom0 under Xen

[Juergen Gross]

Commit 920cf4194954ec ("drm/i915: Introduce an internal allocator for
disposable private objects") introduced a regression for the kernel
running as Xen dom0: when switching to graphics mode a GPU HANG
occurred.

Reason seems to be a missing adaption similar to that done in
commit 7453c549f5f648 ("swiotlb: Export swiotlb_max_segment to users")
to i915_gem_object_get_pages_internal().

So limit the maximum page order to be used according to the maximum
swiotlb segment size instead to the complete swiotlb size.

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
Please consider for 4.10 as otherwise 4.10 will be unusable as Xen dom0
with i915 graphics.
---
 drivers/gpu/drm/i915/i915_gem_internal.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_internal.c 
b/drivers/gpu/drm/i915/i915_gem_internal.c
index 4b3ff3e..d09c749 100644
--- a/drivers/gpu/drm/i915/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/i915_gem_internal.c
@@ -66,8 +66,16 @@ i915_gem_object_get_pages_internal(struct 
drm_i915_gem_object *obj)
 
max_order = MAX_ORDER;
 #ifdef CONFIG_SWIOTLB
-   if (swiotlb_nr_tbl()) /* minimum max swiotlb size is IO_TLB_SEGSIZE */
-   max_order = min(max_order, ilog2(IO_TLB_SEGPAGES));
+   if (swiotlb_nr_tbl()) {
+   unsigned int max_segment;
+
+   max_segment = swiotlb_max_segment();
+   if (max_segment) {
+   max_segment = max_t(unsigned int, max_segment,
+   PAGE_SIZE) >> PAGE_SHIFT;
+   max_order = min(max_order, ilog2(max_segment));
+   }
+   }
 #endif
 
gfp = GFP_KERNEL | __GFP_HIGHMEM | __GFP_RECLAIMABLE;
-- 
2.10.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Re: [Intel-gfx] GPU hang with kernel 4.10rc3

[Juergen Gross]

On 13/01/17 15:41, Juergen Gross wrote:
> On 12/01/17 10:21, Chris Wilson wrote:
>> On Thu, Jan 12, 2017 at 07:03:25AM +0100, Juergen Gross wrote:
>>> On 11/01/17 18:08, Chris Wilson wrote:
>>>> On Wed, Jan 11, 2017 at 05:33:34PM +0100, Juergen Gross wrote:
>>>>> With kernel 4.10rc3 running as Xen dm0 I get at each boot:
>>>>>
>>>>> [   49.213697] [drm] GPU HANG: ecode 7:0:0x3d1d3d3d, in gnome-shell
>>>>> [1431], reason: Hang on render ring, action: reset
>>>>> [   49.213699] [drm] GPU hangs can indicate a bug anywhere in the entire
>>>>> gfx stack, including userspace.
>>>>> [   49.213700] [drm] Please file a _new_ bug report on
>>>>> bugs.freedesktop.org against DRI -> DRM/Intel
>>>>> [   49.213700] [drm] drm/i915 developers can then reassign to the right
>>>>> component if it's not a kernel issue.
>>>>> [   49.213700] [drm] The gpu crash dump is required to analyze gpu
>>>>> hangs, so please always attach it.
>>>>> [   49.213701] [drm] GPU crash dump saved to /sys/class/drm/card0/error
>>>>> [   49.213755] drm/i915: Resetting chip after gpu hang
>>>>> [   60.213769] drm/i915: Resetting chip after gpu hang
>>>>> [   71.189737] drm/i915: Resetting chip after gpu hang
>>>>> [   82.165747] drm/i915: Resetting chip after gpu hang
>>>>> [   93.205727] drm/i915: Resetting chip after gpu hang
>>>>>
>>>>> The dump is attached.
>>>>
>>>> That's a nasty one. The first couple of pages of the batchbuffer appear
>>>> to be overwritten. (Full of 0xc2c2c2c2, i.e. probably pixel data.) That
>>>> may be a concurrent write by either the GPU or CPU, or we may have
>>>> incorrected mapped a set of pages. That it doesn't recovered suggests
>>>> that the corruption occurs frequently, probably on every request/batch.
>>>
>>> I hoped someone would have an idea already.
>>
>> Sorry, first report of something like this in a long time (that I can
>> remember at least). And the problem is that it can be anything from a
>> coherency to a concurrency issue, so no one patch springs to mind.
>> Thankfully it appears to be kernel related.
>> -Chris
>>
> 
> Bisecting took longer than I thought, but I had to cherry pick some
> patches and rebase one of them multiple times...
> 
> Finally I found the commit to blame: 920cf4194954ec ("drm/i915:
> Introduce an internal allocator for disposable private objects")
> 
> In case you need me to produce some more data or test a patch
> feel free to reach out.

Anything new for this severe regression?

Without a fix 4.10 will be unusable with Xen on a machine with i915
graphics!


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] GPU hang with kernel 4.10rc3

[Juergen Gross]

On 12/01/17 10:21, Chris Wilson wrote:
> On Thu, Jan 12, 2017 at 07:03:25AM +0100, Juergen Gross wrote:
>> On 11/01/17 18:08, Chris Wilson wrote:
>>> On Wed, Jan 11, 2017 at 05:33:34PM +0100, Juergen Gross wrote:
>>>> With kernel 4.10rc3 running as Xen dm0 I get at each boot:
>>>>
>>>> [   49.213697] [drm] GPU HANG: ecode 7:0:0x3d1d3d3d, in gnome-shell
>>>> [1431], reason: Hang on render ring, action: reset
>>>> [   49.213699] [drm] GPU hangs can indicate a bug anywhere in the entire
>>>> gfx stack, including userspace.
>>>> [   49.213700] [drm] Please file a _new_ bug report on
>>>> bugs.freedesktop.org against DRI -> DRM/Intel
>>>> [   49.213700] [drm] drm/i915 developers can then reassign to the right
>>>> component if it's not a kernel issue.
>>>> [   49.213700] [drm] The gpu crash dump is required to analyze gpu
>>>> hangs, so please always attach it.
>>>> [   49.213701] [drm] GPU crash dump saved to /sys/class/drm/card0/error
>>>> [   49.213755] drm/i915: Resetting chip after gpu hang
>>>> [   60.213769] drm/i915: Resetting chip after gpu hang
>>>> [   71.189737] drm/i915: Resetting chip after gpu hang
>>>> [   82.165747] drm/i915: Resetting chip after gpu hang
>>>> [   93.205727] drm/i915: Resetting chip after gpu hang
>>>>
>>>> The dump is attached.
>>>
>>> That's a nasty one. The first couple of pages of the batchbuffer appear
>>> to be overwritten. (Full of 0xc2c2c2c2, i.e. probably pixel data.) That
>>> may be a concurrent write by either the GPU or CPU, or we may have
>>> incorrected mapped a set of pages. That it doesn't recovered suggests
>>> that the corruption occurs frequently, probably on every request/batch.
>>
>> I hoped someone would have an idea already.
> 
> Sorry, first report of something like this in a long time (that I can
> remember at least). And the problem is that it can be anything from a
> coherency to a concurrency issue, so no one patch springs to mind.
> Thankfully it appears to be kernel related.
> -Chris
> 

Bisecting took longer than I thought, but I had to cherry pick some
patches and rebase one of them multiple times...

Finally I found the commit to blame: 920cf4194954ec ("drm/i915:
Introduce an internal allocator for disposable private objects")

In case you need me to produce some more data or test a patch
feel free to reach out.


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] GPU hang with kernel 4.10rc3

[Juergen Gross]

On 11/01/17 18:08, Chris Wilson wrote:
> On Wed, Jan 11, 2017 at 05:33:34PM +0100, Juergen Gross wrote:
>> With kernel 4.10rc3 running as Xen dm0 I get at each boot:
>>
>> [   49.213697] [drm] GPU HANG: ecode 7:0:0x3d1d3d3d, in gnome-shell
>> [1431], reason: Hang on render ring, action: reset
>> [   49.213699] [drm] GPU hangs can indicate a bug anywhere in the entire
>> gfx stack, including userspace.
>> [   49.213700] [drm] Please file a _new_ bug report on
>> bugs.freedesktop.org against DRI -> DRM/Intel
>> [   49.213700] [drm] drm/i915 developers can then reassign to the right
>> component if it's not a kernel issue.
>> [   49.213700] [drm] The gpu crash dump is required to analyze gpu
>> hangs, so please always attach it.
>> [   49.213701] [drm] GPU crash dump saved to /sys/class/drm/card0/error
>> [   49.213755] drm/i915: Resetting chip after gpu hang
>> [   60.213769] drm/i915: Resetting chip after gpu hang
>> [   71.189737] drm/i915: Resetting chip after gpu hang
>> [   82.165747] drm/i915: Resetting chip after gpu hang
>> [   93.205727] drm/i915: Resetting chip after gpu hang
>>
>> The dump is attached.
> 
> That's a nasty one. The first couple of pages of the batchbuffer appear
> to be overwritten. (Full of 0xc2c2c2c2, i.e. probably pixel data.) That
> may be a concurrent write by either the GPU or CPU, or we may have
> incorrected mapped a set of pages. That it doesn't recovered suggests
> that the corruption occurs frequently, probably on every request/batch.

I hoped someone would have an idea already.

> Is this a new bug? Bisection would be the fastest way to triage it.

Commit 7453c549f was still okay. Starting bisect now (2882 commits, 12
steps) ...


Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

GPU hang with kernel 4.10rc3

[Juergen Gross]

With kernel 4.10rc3 running as Xen dm0 I get at each boot:

[   49.213697] [drm] GPU HANG: ecode 7:0:0x3d1d3d3d, in gnome-shell
[1431], reason: Hang on render ring, action: reset
[   49.213699] [drm] GPU hangs can indicate a bug anywhere in the entire
gfx stack, including userspace.
[   49.213700] [drm] Please file a _new_ bug report on
bugs.freedesktop.org against DRI -> DRM/Intel
[   49.213700] [drm] drm/i915 developers can then reassign to the right
component if it's not a kernel issue.
[   49.213700] [drm] The gpu crash dump is required to analyze gpu
hangs, so please always attach it.
[   49.213701] [drm] GPU crash dump saved to /sys/class/drm/card0/error
[   49.213755] drm/i915: Resetting chip after gpu hang
[   60.213769] drm/i915: Resetting chip after gpu hang
[   71.189737] drm/i915: Resetting chip after gpu hang
[   82.165747] drm/i915: Resetting chip after gpu hang
[   93.205727] drm/i915: Resetting chip after gpu hang

The dump is attached.


Juergen
GPU HANG: ecode 7:0:0x3d1d3d3d, in gnome-shell [1431], reason: Hang on render 
ring, action: reset
Kernel: 4.10.0-rc3-pv+
Time: 1484151498 s 569085 us
Boottime: 39 s 844107 us
Uptime: 34 s 750060 us
is_mobile: no
is_i85x: no
is_i915g: no
is_i945gm: no
is_g33: no
is_g4x: no
is_pineview: no
is_broadwater: no
is_crestline: no
is_ivybridge: no
is_valleyview: no
is_cherryview: no
is_haswell: yes
is_broadwell: no
is_skylake: no
is_broxton: no
is_kabylake: no
is_alpha_support: no
has_64bit_reloc: no
has_csr: no
has_ddi: yes
has_dp_mst: yes
has_fbc: yes
has_fpga_dbg: yes
has_gmbus_irq: yes
has_gmch_display: no
has_guc: no
has_hotplug: yes
has_hw_contexts: yes
has_l3_dpf: yes
has_llc: yes
has_logical_ring_contexts: no
has_overlay: no
has_pipe_cxsr: no
has_pooled_eu: no
has_psr: yes
has_rc6: yes
has_rc6p: no
has_resource_streamer: yes
has_runtime_pm: yes
has_snoop: no
cursor_needs_physical: no
hws_needs_physical: no
overlay_needs_physical: no
supports_tv: no
has_decoupled_mmio: no
Active process (on ring render): gnome-shell [1431]
Reset count: 0
Suspend count: 0
PCI ID: 0x0416
PCI Revision: 0x06
PCI Subsystem: 1028:05bd
IOMMU enabled?: 0
EIR: 0x
IER: 0xfc002529
GTIER: 0x00401821
PGTBL_ER: 0x
FORCEWAKE: 0x0001
DERRMR: 0x
CCID: 0x012f710d
Missed interrupts: 0x
  fence[0] = bbf03300603001
  fence[1] = c3300700bf4003
  fence[2] = d3300f00c34003
  fence[3] = 12f003300d34001
  fence[4] = 230703f01308003
  fence[5] = 2b0003b02309003
  fence[6] = 30c503302b09001
  fence[7] = 
  fence[8] = 
  fence[9] = 
  fence[10] = 
  fence[11] = 
  fence[12] = 
  fence[13] = 
  fence[14] = 
  fence[15] = 
  fence[16] = 
  fence[17] = 
  fence[18] = 
  fence[19] = 
  fence[20] = 
  fence[21] = 
  fence[22] = 
  fence[23] = 
  fence[24] = 
  fence[25] = 
  fence[26] = 
  fence[27] = 
  fence[28] = 
  fence[29] = 
  fence[30] = 
  fence[31] = 
ERROR: 0x0101
DONE_REG: 0xffef
ERR_INT: 0x
render command stream:
  START: 0x1000
  HEAD:  0x0620 [0x05f8]
  TAIL:  0x0668 [0x0630, 0x0668]
  CTL:   0x0001f001
  MODE:  0x4000
  HWS:   0x7fff
  ACTHD: 0x 030c9004
  IPEIR: 0x
  IPEHR: 0xc2c2c2c2
  INSTDONE: 0xffdf
  SC_INSTDONE: 0x
  SAMPLER_INSTDONE[0][0]: 0x
  ROW_INSTDONE[0][0]: 0x
  batch: [0x_030c9000, 0x_030cc000]
  BBADDR: 0x_030c9005
  BB_STATE: 0x
  INSTPS: 0x8201
  INSTPM: 0x6080
  FADDR: 0x 030c9200
  RC PSMI: 0x0010
  FAULT_REG: 0x00c5
  SYNC_0: 0x
  SYNC_1: 0x0002
  SYNC_2: 0x
  GFX_MODE: 0x2a00
  PP_DIR_BASE: 0x7fdf
  seqno: 0x0008
  last_seqno: 0x0009
  waiting: yes
  ring->head: 0x
  ring->tail: 0x0668
  hangcheck: hung [42]
blt command stream:
  START: 0x00022000
  HEAD:  0x0088 [0x]
  TAIL:  0x0088 [0x, 0x]
  CTL:   0x0001f001
  MODE:  0x0200
  HWS:   0x00021000
  ACTHD: 0x 0088
  IPEIR: 0x
  IPEHR: 0x
  INSTDONE: 0xfffe
  BBADDR: 0x_00bc0024
  BB_STATE: 0x
  INSTPS: 0x
  INSTPM: 0x
  FADDR: 0x 00022088
  RC PSMI: 0x0010
  FAULT_REG: 0x
  SYNC_0: 0x0008
  SYNC_1: 0x
  SYNC_2: 0x
  GFX_MODE: 0x0200
  PP_DIR_BASE: 0x7fdf
  seqno: 0x0002
  last_seqno: 0x0002
  waiting: no
  ring->head: 0x
  ring->tail: 0x
  hangcheck: idle [0]
bsd command stream:
  START: 0x00043000
  HEAD:  0x [0x]
  TAIL:  0x [0x, 0x]
  CTL:   0x0001f001
  MODE:  0x0200
  HWS:   0x00042000
  ACTHD: 0x 
  IPEIR: 0x
  IPEHR: 0x
  INSTDONE: 0xfffe
  BBADDR: 0x_
  BB_STATE: 0x
  INSTPS: 0x
  INSTPM: 0x
 

i915 regression in kernel 4.10

[Juergen Gross]

On 19/12/16 13:29, Chris Wilson wrote:
> On Mon, Dec 19, 2016 at 12:39:16PM +0100, Juergen Gross wrote:
>> With recent 4.10 kernel the graphics isn't coming up under Xen. First
>> failure message is:
>>
>> [   46.656649] i915 :00:02.0: swiotlb buffer is full (sz: 1630208 bytes)
> 
> Do we get a silent failure? i915_gem_gtt_prepare_pages() is where we
> call dma_map_sg() and pass the sg to swiotlb (in this case) for
> remapping, and we do check for an error value of 0. After that error,
> SWIOTLB_MAP_ERROR is propagated back and converted to 0 for
> dma_map_sg(). That looks valid, and we should report ENOMEM back to the
> caller.
> 
>> Later I see splats like:
>>
>> [   49.393583] general protection fault:  [#1] SMP
> 
> What was the faulting address? RAX is particularly non-pointer-like so I
> wonder if we walked onto an uninitialised portion of the sgtable. We may
> have tripped over a bug in our sg_page iterator.

During the bisect process there have been either GP or NULL pointer
dereferences or other page faults. Typical addresses where:

xen_swiotlb_unmap_sg_attrs+0x1f/0x50: access to 0018
xen_swiotlb_unmap_sg_attrs+0x1f/0x50: access to 03020118

> 
> The attached patch should prevent an early ENOMEM following the swiotlb
> allocation failure. But I suspect that we will still be tripping up the
> failure in the sg walker when binding to the GPU.
> -Chris
> 

The patch is working not too bad. :-)

Still several "swiotlb buffer is full" messages (some with sz:, most
without), but no faults any more (neither GP nor NULL pointer
dereference). Graphical login is working now.

What I do see, however, is (no idea whether this is related):

[  735.826492] INFO: task systemd-udevd:484 blocked for more than 120
seconds.
[  735.826497]   Tainted: GW   4.9.0-pv+ #767
[  735.826499] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
[  735.826501] systemd-udevd   D0   484443 0x
[  735.826507] Call Trace:
[  735.826522]  ? __schedule+0x192/0x640
[  735.826530]  ? kmem_cache_free+0x45/0x150
[  735.826535]  ? schedule+0x2d/0x80
[  735.826539]  ? schedule_timeout+0x1f3/0x380
[  735.826545]  ? error_exit+0x9/0x20
[  735.826555]  ? sg_pool_index.part.0+0x2/0x2
[  735.826561]  ? wait_for_completion+0xa4/0x110
[  735.826569]  ? wake_up_q+0x70/0x70
[  735.826577]  ? cpufreq_boost_online+0x10/0x10 [acpi_cpufreq]
[  735.826585]  ? cpuhp_issue_call+0x9c/0xe0
[  735.826590]  ? __cpuhp_setup_state+0xd5/0x1d0
[  735.826599]  ? acpi_cpufreq_init+0x1cd/0x1000 [acpi_cpufreq]
[  735.826601]  ? 0xa00b1000
[  735.826607]  ? do_one_initcall+0x38/0x180
[  735.826611]  ? kmem_cache_alloc_trace+0x98/0x1e0
[  735.826620]  ? do_init_module+0x55/0x1e5
[  735.826629]  ? load_module+0x2088/0x26b0
[  735.826633]  ? __symbol_put+0x30/0x30
[  735.826639]  ? SYSC_finit_module+0x80/0xb0
[  735.826644]  ? entry_SYSCALL_64_fastpath+0x1e/0xad

I guess it is _not_ related, OTOH there is sg_pool_index() involved...


Juergen

i915 regression in kernel 4.10

[Juergen Gross]

With recent 4.10 kernel the graphics isn't coming up under Xen. First
failure message is:

[   46.656649] i915 :00:02.0: swiotlb buffer is full (sz: 1630208 bytes)

Later I see splats like:

[   49.393583] general protection fault:  [#1] SMP
[   49.403800] Modules linked in: bridge stp llc tun arc4 iwldvm
mac80211 uvcvideo snd_hda_codec_realtek snd_hda_codec_hdmi
videobuf2_vmalloc snd_hda_codec_generic videobuf2_memops snd_hda_intel
videobuf2_v4l2 snd_hda_codec videobuf2_core iwlwifi videodev
snd_hda_core e1000e intel_rapl xhci_pci x86_pkg_temp_thermal xhci_hcd
intel_powerclamp coretemp crct10dif_pclmul snd_hwdep joydev crc32_pclmul
crc32c_intel ghash_clmulni_intel mei_me ptp iTCO_wdt aesni_intel snd_pcm
ppdev mei aes_x86_64 iTCO_vendor_support lrw sdhci_pci gf128mul lpc_ich
parport_pc pps_core cfg80211 glue_helper ablk_helper snd_timer parport
mfd_core i2c_i801 snd i2c_smbus dell_laptop i2c_hid hid psmouse
dell_rbtn dell_smbios tpm_tis i2c_designware_platform rfkill soundcore
xenfs tpm_tis_core i2c_designware_core tpm dcdbas xen_privcmd evdev
serio_raw battery ac dell_smm_hwmon thermal dell_smo8800 cryptd pcspkr
dm_mod ext4 crc16 jbd2 mbcache sr_mod cdrom sd_mod ahci libahci ehci_pci
ehci_hcd libata usbcore usb_common sdhci_acpi sdhci i915 video mmc_core
i2c_algo_bit drm_kms_helper drm button xen_acpi_processor xen_pciback
xen_netback xen_blkback xen_gntalloc xen_gntdev xen_evtchn
target_core_mod configfs sg scsi_mod autofs4
[   49.616634] CPU: 2 PID: 1663 Comm: gnome-shell Tainted: GW
   4.8.0-rc2-pv+ #763
[   49.633989] Hardware name: Dell Inc. Latitude E6440/0159N7, BIOS A07
06/26/2014
[   49.649235] task: 8801f44ac140 task.stack: 8801f944c000
[   49.661738] RIP: e030:[]  []
gen6_ppgtt_insert_entries+0xd6/0x1f0 [i915]
[   49.682062] RSP: e02b:8801f944faa0  EFLAGS: 00010202
[   49.693355] RAX: 672f726574697254 RBX: 8801f46beda0 RCX:

[   49.708282] RDX: 672f726574697257 RSI: 0001 RDI:
0640
[   49.723232] RBP: 03020140 R08:  R09:
8801f46e2000
[   49.738182] R10: 8801f944fc00 R11: 0007 R12:
1031
[   49.753110] R13: 8801f46e2000 R14: 8801f46be000 R15:
0369
[   49.768039] FS:  7f0e354e5a80() GS:8801ff90()
knlGS:8801ff90
[   49.784879] CS:  e033 DS:  ES:  CR0: 80050033
[   49.797033] CR2: 05636ff0 CR3: 0001e9c8c000 CR4:
00042660
[   49.811972] Stack:
[   49.816681]  727365642f61632f 00010031 0001
8801f44ac140
[   49.832131]  8801f45f7a00 0080 0001

[   49.847584]  8801f944fcde 8801f45f7a00 a01f7333
eb9bbc88
[   49.863045] Call Trace:
[   49.868652]  [] ? aliasing_gtt_bind_vma+0x83/0xd0
[i915]
[   49.883111]  [] ? i915_vma_bind+0x79/0x110 [i915]
[   49.896345]  [] ? __i915_vma_do_pin+0x2ff/0x4c0 [i915]
[   49.910477]  [] ? __radix_tree_insert+0x29/0xc0
[   49.923368]  [] ? kmem_cache_alloc+0x1c4/0x1e0
[   49.936159]  [] ?
i915_gem_execbuffer_reserve_vma.isra.43+0x131/0x1a0 [i915]
[   49.954117]  [] ?
i915_gem_execbuffer_reserve.isra.44+0x2aa/0x3a0 [i915]
[   49.971352]  [] ?
i915_gem_do_execbuffer.isra.49+0x6a9/0x1690 [i915]
[   49.987889]  [] ? __alloc_pages_nodemask+0x133/0xc40
[   50.001677]  [] ? i915_gem_execbuffer2+0xd6/0x240
[i915]
[   50.016131]  [] ? drm_ioctl+0x191/0x3f0 [drm]
[   50.028675]  [] ? i915_gem_execbuffer+0x320/0x320
[i915]
[   50.043123]  [] ? handle_mm_fault+0x3ac/0x12e0
[   50.055839]  [] ? do_vfs_ioctl+0x8a/0x5b0
[   50.067677]  [] ? SyS_ioctl+0x6f/0x80
[   50.078815]  [] ? entry_SYSCALL_64_fastpath+0x1e/0xa8
[   50.092725] Code: 00 10 00 00 44 3b 64 24 08 72 ae f6 45 00 02 75 66
48 8b 55 20 48 8d 45 20 f6 c2 01 0f 85 f8 00 00 00 48 85 c0 0f 84 fb 00
00 00 <44> 8b 60 08 8b 48 0c 48 89 c5 48 8b 70 10 44 01 e1 44 89 e7 48
[   50.131694] RIP  []
gen6_ppgtt_insert_entries+0xd6/0x1f0 [i915]
[   50.147396]  RSP 
[   50.158793] ---[ end trace 6fc5bf1c09423b3f ]---

I have bisected the commits and found the bug to be introduced in
commit 871dfbd67d4ecbcc83fc9e80a310ca9bf3c44c40 ("drm/i915: Allow
compaction upto SWIOTLB max segment size").

To be able to test this commit with Xen you'll need the attached patch
in order to enable the system to boot or you have to disable loading
of microcode.


Juergen
-- next part --
A non-text attachment was scrubbed...
Name: boris.patch
Type: text/x-patch
Size: 3759 bytes
Desc: not available
URL: