Re: [Xen-devel] PV DRM doesn't work without auto_translated_physmap feature in Dom0
Hi, On 4/19/20 14:26, Santucco wrote: > Hello, > I have found a source of the problem. > In displ_be, BaseDump copies to the drm buffer with a size from > i915 drm driver, but this size a bit more than a size of my frontend > display buffer. I have made a quick and dirty fix, a copy a line of my > display buffer to a middle of a line of the drm display buffer. Patch > is attached. Thank you for the patch and your efforts to fix the issue. Could you please make a pull request to [1], so we can continue there? Thank you in advance, Oleksandr > Best regards, > Alexander > > Четверг, 6 февраля 2020, 11:20 +03:00 от Oleksandr Andrushchenko > : > On 2/5/20 8:59 PM, Santucco wrote: > > Hello, > > Ok, I commented out the memcpy call and run the test. > > displ_be hasn’t crached, I have seen FLIP events in the log. > > But there hasn’t been the black screen, just a blink effect every > > couple of seconds. > > Logs are attached. > Ok, so I believe that frontend - backend (displ_be) communication > is ok > and there is nothing to do there. > > Next, I would start debugging the following in Xen: > (XEN) mm.c:2223:d2v0 Bad L1 flags 80 > and have a look at [1]. Probably, someone on Xen x86 side can tell > if this could be related to the flags at [2]. > > > Best regards, > > Alexander > > > > Среда, 5 февраля 2020, 9:31 +03:00 от Oleksandr Andrushchenko > > >: > > On 2/4/20 10:28 AM, Santucco wrote: > > > Hello, > > > displ_be was compiled without zero-copy support early. > > > I have tried with the recompiled dom0 kernel, a result is the > same. > > > Logs and configs (+displ_be’s CMakeCache.txt ) are attached. > > Ok, yet another test to localize the problem. > > Could you please remove memcpy from > > #1 0x55e5a1f28bec in Drm::DumbDrm::copy > (this=0x7f9338000e00) at > > > /home/santucco/tmp/xen-troops/displ_be/src/displayBackend/drm/Dumb.cpp:149 > > and just memset the destination with 0 or whatever. > > > > I expect that system won't crash, nothing will be shown (black > > screen), but > > displ_be will show page flip events in its logs. > > > Best regards, > > > Alexander > > > > > > Понедельник, 3 февраля 2020, 10:36 +03:00 от Oleksandr > > > Andrushchenko > > >: > > > > > > > > > On 2/1/20 4:39 PM, Santucco wrote: > > > > Hello again, > > > > I have not yet made to work my drm client, so I have tried > to run > > > > linux like a domU (to see how it should work), it doesn’t > work too > > > > — displ_be catches SIGSEGV: > > > > > > > > #0 0x7f4afed1c161 in ?? () from /lib64/libc.so.6 > > > > #1 0x55723b9c5bec in Drm::DumbDrm::copy > > > (this=0x7f4adc000e00) at > > > > > > > > > > /home/santucco/tmp/xen-troops/displ_be/src/displayBackend/drm/Dumb.cpp:149 > > > > #2 0x55723b9a8f51 in BuffersStorage::getFrameBufferAndCopy > > > > (this=0x7f4ae00010e0, fbCookie=18446612682295083264) at > > > > > > > > > > > /home/santucco/tmp/xen-troops/displ_be/src/displayBackend/BuffersStorage.cpp:165 > > > > It tries to copy to mBuffer with non-accessible address. > > > > For the moment I see a strange offset for mmap call of > > > /dev/drm/card0 > > > > in the strace log — 0x1. Is that normal? > > > > Any direction of which to dig will be very helpful. > > > > Configuration details: > > > > Xen 4.12.1 Dom0: Linux 4.20.17-gentoo #13 SMP Sat Dec 28 > > > 11:12:24 MSK > > > > 2019 x86_64 Intel(R) Celeron(R) CPU N3050 @ 1.60GHz GenuineIntel > > > GNU/Linux > > > > DomU: Linux 4.20.17-gentoo > > > > last xen-troops/libxenbe and xen-troops/displ_be > > > > Logs (dmesg, xl dmesg, displ_be, strace log of displ_be), a > > > backtrace > > > > of gdb and kernel configs are attached. > > > > Thanks in advance. > > > Could you please try Dom0 kernel WITHOUT the options below: > > > CONFIG_XEN_GNTDEV_DMABUF=y > > > CONFIG_XEN_GRANT_DMA_ALLOC=y > > > > > > Then, just to make sure, did you build displ_be without zero-copy > > > support? > > > > > > > On 1/8/20 5:38 PM, Santucco wrote: > > > > > Thank you very much for all your answers. > > > > > > > > > > Среда, 8 января 2020, 10:54 +03:00 от Oleksandr Andrushchenko > > > > > > > > > > > > >: > > > > > On 1/6/20 10:38 AM, Jürgen Groß wrote: > > > > > > On 06.01.20 08:56, Santucco wrote: > > > > > >> Hello, > > > > > >> > > > > > >> I’m trying to use vdispl interface from PV OS, it doesn’t > > work. > > > > > >> Configuration details: > > > > > >> Xen 4.12.1 > > > > > >> Dom0: Linux 4.20.17-gentoo #13 SMP Sat Dec 28 > > 11:12:24 MSK > > > > > 2019 > > > > > >> x86_64 Intel(R) Celeron(R) CPU N3050 @
Re: [PATCH 01/10] x86/mm: no-one passes a NULL domain to init_xen_l4_slots()
On 17.04.2020 21:46, Andrew Cooper wrote:
> On 17/04/2020 15:25, Jan Beulich wrote:
>> Drop the NULL checks - they've been introduced by commit 8d7b633ada
>> ("x86/mm: Consolidate all Xen L4 slot writing into
>> init_xen_l4_slots()") for no apparent reason.
>
> :) I'll take this as conformation that all my sudden pagetable work in
> Xen manage ended up being rather more subtle than Linux's equivalent
> work for KPTI.
>
> https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00281.html
>
> Specifically, this was part of trying to arrange for fully per-pcpu
> private mappings, and was used to construct the pagetables for the idle
> vcpu which specifically don't have a perdomain mapping.
>
> Seeing as this is still an outstanding task in the secret-free-Xen
> plans, any dropping of it now will have to be undone at some point in
> the future.
s/will/may/ I suppose - we don't know for sure how this will look
like at this point.
> Is there a specific reason for the cleanup?
Elimination of effectively dead code. We avoid arbitrary NULL checks
elsewhere as well; iirc I've seen you (amongst others) comment on
people trying to introduce such in their patches.
>> Signed-off-by: Jan Beulich
>>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -1696,7 +1696,7 @@ void init_xen_l4_slots(l4_pgentry_t *l4t
>
> If we continue with this patch, this comment, just out of context, needs
> adjusting.
Will do.
Jan
RE: [PATCH 1/1] x86/vtd: Mask DMAR faults for IGD devices
> From: Brendan Kerrigan
> Sent: Friday, April 17, 2020 9:36 PM
>
> From: Brendan Kerrigan
>
> The Intel graphics device records DMAR faults regardless
> of the Fault Process Disable bit being set. When this fault
Since this is an errata for specific generations, let's describe
this way instead of stating it as a generic problem.
> occurs, enable the Interrupt Mask (IM) bit in the Fault
> Event Control (FECTL) register to prevent the continued
> recording of the fault.
>
> Signed-off-by: Brendan Kerrigan
> ---
> xen/drivers/passthrough/vtd/iommu.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 07d40b37fe..288399d816 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -41,6 +41,8 @@
> #include "vtd.h"
> #include "../ats.h"
>
> +#define IS_IGD(seg, id) (0 == seg && 0 == PCI_BUS(id) && 2 == PCI_SLOT(id)
> && 0 == PCI_FUNC(id))
> +
> struct mapped_rmrr {
> struct list_head list;
> u64 base, end;
> @@ -872,6 +874,13 @@ static int iommu_page_fault_do_one(struct
> vtd_iommu *iommu, int type,
> printk(XENLOG_G_WARNING VTDPREFIX "%s: reason %02x - %s\n",
> kind, fault_reason, reason);
>
> +if ( DMA_REMAP == fault_type && type && IS_IGD(seg, source_id) ) {
> +u32 fectl = dmar_readl(iommu->reg, DMAR_FECTL_REG);
> +fectl |= DMA_FECTL_IM;
> +dmar_writel(iommu->reg, DMAR_FECTL_REG, fectl);
> +printk(XENLOG_G_WARNING VTDPREFIX "Disabling DMAR faults for
> IGD\n");
> +}
> +
Several questions. First, I just note that FPD is not touched by any code
today. then how is this errata being caught? Second, we already have
pci_check_disable_device in place which stops DMAs from the problematic
device if it triggers excessive fault reports. why doesn't it work for your
case? Last, dma_msi_end just forces clearing the mask bit at end of handling
the fault interrupt, making above fix meaningless. Those questions just
make me wonder how you actually test this patch and whether it's necessary...
Thanks
Kevin
> if ( iommu_verbose && fault_type == DMA_REMAP )
> print_vtd_entries(iommu, PCI_BUS(source_id), PCI_DEVFN2(source_id),
>addr >> PAGE_SHIFT);
> --
> 2.17.1
RE: [PATCH] x86/vtd: relax EPT page table sharing check
> From: Roger Pau Monne > Sent: Friday, April 17, 2020 7:30 PM > > The EPT page tables can be shared with the IOMMU as long as the page > sizes supported by EPT are also supported by the IOMMU. > > Current code checks that both the IOMMU and EPT support the same page > sizes, but this is not strictly required, the IOMMU supporting more > page sizes than EPT is fine and shouldn't block page table sharing. > > This is likely not a common case (IOMMU supporting more page sizes > than EPT), but should still be fixed for correctness. > > Signed-off-by: Roger Pau Monné Reviewed-by: Kevin Tian
[PATCH] pvcalls: Document explicitly the padding for all arches
From: Julien Grall
The documentation of pvcalls only describes the padding for i386. This
is a bit odd as there are some implicit padding for 64-bit (e.g in
xen_pvcalls_release) and this doesn't cater other 32-bit arch.
Remove the #ifdef in the documentation to show the padding is present on
all arches and take the opportunity to describe the padding in the
public header as well.
Signed-off-by: Julien Grall
---
AFAICT, we cannot mandate the padding to be 0 as they are already
present.
---
docs/misc/pvcalls.pandoc| 8
xen/include/public/io/pvcalls.h | 4
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
index 665dad556c..971cd8f4b1 100644
--- a/docs/misc/pvcalls.pandoc
+++ b/docs/misc/pvcalls.pandoc
@@ -246,9 +246,7 @@ The format is defined as follows:
uint32_t domain;
uint32_t type;
uint32_t protocol;
- #ifdef CONFIG_X86_32
uint8_t pad[4];
- #endif
} socket;
struct xen_pvcalls_connect {
uint64_t id;
@@ -257,16 +255,12 @@ The format is defined as follows:
uint32_t flags;
grant_ref_t ref;
uint32_t evtchn;
- #ifdef CONFIG_X86_32
uint8_t pad[4];
- #endif
} connect;
struct xen_pvcalls_release {
uint64_t id;
uint8_t reuse;
- #ifdef CONFIG_X86_32
uint8_t pad[7];
- #endif
} release;
struct xen_pvcalls_bind {
uint64_t id;
@@ -276,9 +270,7 @@ The format is defined as follows:
struct xen_pvcalls_listen {
uint64_t id;
uint32_t backlog;
- #ifdef CONFIG_X86_32
uint8_t pad[4];
- #endif
} listen;
struct xen_pvcalls_accept {
uint64_t id;
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index cb8171275c..f6048ddc63 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -65,6 +65,7 @@ struct xen_pvcalls_request {
uint32_t domain;
uint32_t type;
uint32_t protocol;
+uint8_t pad[4];
} socket;
struct xen_pvcalls_connect {
uint64_t id;
@@ -73,10 +74,12 @@ struct xen_pvcalls_request {
uint32_t flags;
grant_ref_t ref;
uint32_t evtchn;
+uint8_t pad[4];
} connect;
struct xen_pvcalls_release {
uint64_t id;
uint8_t reuse;
+uint8_t pad[7];
} release;
struct xen_pvcalls_bind {
uint64_t id;
@@ -86,6 +89,7 @@ struct xen_pvcalls_request {
struct xen_pvcalls_listen {
uint64_t id;
uint32_t backlog;
+uint8_t pad[4];
} listen;
struct xen_pvcalls_accept {
uint64_t id;
--
2.17.1
[PATCH] xen/arm: Avoid to open-code the relinquish state machine
In commit 0dfffe01d5 "x86: Improve the efficiency of
domain_relinquish_resources()", the x86 version of the function has been
reworked to avoid open-coding the state machine and also add more
documentation.
Bring the Arm version on part with x86 by introducing a documented
PROGRESS() macro to avoid latent bugs and make the new PROG_* states
private to domain_relinquish_resources().
Cc: Andrew Cooper
Signed-off-by: Julien Grall
---
xen/arch/arm/domain.c| 60 ++--
xen/include/asm-arm/domain.h | 9 +-
2 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6627be2922..31169326b2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -674,7 +674,6 @@ int arch_domain_create(struct domain *d,
int rc, count = 0;
BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
-d->arch.relmem = RELMEM_not_started;
/* Idle domains do not need this setup */
if ( is_idle_domain(d) )
@@ -950,13 +949,41 @@ static int relinquish_memory(struct domain *d, struct
page_list_head *list)
return ret;
}
+/*
+ * Record the current progress. Subsequent hypercall continuations will
+ * logically restart work from this point.
+ *
+ * PROGRESS() markers must not be in the middle of loops. The loop
+ * variable isn't preserved accross a continuation.
+ *
+ * To avoid redundant work, there should be a marker before each
+ * function which may return -ERESTART.
+ */
+enum {
+PROG_tee = 1,
+PROG_xen,
+PROG_page,
+PROG_mapping,
+PROG_done,
+};
+
+#define PROGRESS(x) \
+d->arch.rel_priv = PROG_ ## x; \
+/* Fallthrough */ \
+case PROG_ ## x
+
int domain_relinquish_resources(struct domain *d)
{
int ret = 0;
-switch ( d->arch.relmem )
+/*
+ * This hypercall can take minutes of wallclock time to complete. This
+ * logic implements a co-routine, stashing state in struct domain across
+ * hypercall continuation boundaries.
+ */
+switch ( d->arch.rel_priv )
{
-case RELMEM_not_started:
+case 0:
ret = iommu_release_dt_devices(d);
if ( ret )
return ret;
@@ -967,42 +994,27 @@ int domain_relinquish_resources(struct domain *d)
*/
domain_vpl011_deinit(d);
-d->arch.relmem = RELMEM_tee;
-/* Fallthrough */
-
-case RELMEM_tee:
+PROGRESS(tee):
ret = tee_relinquish_resources(d);
if (ret )
return ret;
-d->arch.relmem = RELMEM_xen;
-/* Fallthrough */
-
-case RELMEM_xen:
+PROGRESS(xen):
ret = relinquish_memory(d, >xenpage_list);
if ( ret )
return ret;
-d->arch.relmem = RELMEM_page;
-/* Fallthrough */
-
-case RELMEM_page:
+PROGRESS(page):
ret = relinquish_memory(d, >page_list);
if ( ret )
return ret;
-d->arch.relmem = RELMEM_mapping;
-/* Fallthrough */
-
-case RELMEM_mapping:
+PROGRESS(mapping):
ret = relinquish_p2m_mapping(d);
if ( ret )
return ret;
-d->arch.relmem = RELMEM_done;
-/* Fallthrough */
-
-case RELMEM_done:
+PROGRESS(done):
break;
default:
@@ -1012,6 +1024,8 @@ int domain_relinquish_resources(struct domain *d)
return 0;
}
+#undef PROGRESS
+
void arch_dump_domain_info(struct domain *d)
{
p2m_dump_info(d);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d39477a939..d2142c6707 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -56,14 +56,7 @@ struct arch_domain
struct vmmio vmmio;
/* Continuable domain_relinquish_resources(). */
-enum {
-RELMEM_not_started,
-RELMEM_tee,
-RELMEM_xen,
-RELMEM_page,
-RELMEM_mapping,
-RELMEM_done,
-} relmem;
+unsigned int rel_priv;
struct {
uint64_t offset;
--
2.17.1
