[ovmf test] 152329: all pass - PUSHED

[osstest service owner]

flight 152329 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152329/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 9001b750df64b25b14ec45a2efa1361a7b96c00a
baseline version:
 ovmf 7f79b736b0a57da71d87c987357db0227cd16ac6

Last test of basis   152315  2020-07-31 03:12:00 Z1 days
Testing same since   152329  2020-07-31 15:40:08 Z0 days1 attempts


People who touched revisions under test:
  Laszlo Ersek 
  Leif Lindholm 
  Rebecca Cran 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   7f79b736b0..9001b750df  9001b750df64b25b14ec45a2efa1361a7b96c00a -> 
xen-tested-master

[qemu-mainline test] 152324: regressions - FAIL

[osstest service owner]

flight 152324 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152324/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-qemuu-nested-intel 14 xen-boot/l1   fail REGR. vs. 151065
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 151065
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 151065
 test-amd64-amd64-qemuu-nested-amd 14 xen-boot/l1 fail REGR. vs. 151065
 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install  fail REGR. vs. 151065
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install  fail REGR. vs. 151065
 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install   fail REGR. vs. 151065
 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install   fail REGR. vs. 151065

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 151065
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 151065
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass

version targeted for testing:
 qemuu1448629751871c4924c234c2faaa968fc26890e1
baseline version:
 qemuu9e3903136d9acde2fb2dd9e967ba928050a6cb4a

Last test of basis   151065  2020-06-12 22:27:51 Z   49 days
Failing since151101  2020-06-14 08:32:51 Z   47 days   67 attempts
Testing same since   152309  2020-07-30 21:40:33 Z1 days2 attempts


People who touched revisions under test:
  Aaron Lindsay 
  Ahmed Karaman 
  Alberto Garcia 
  Aleksandar Markovic 
  Aleksandar Markovic 
  Alex Bennée 
  Alex Richardson 
  Alex Williamson 
  Alexander Boettcher 
  Alexander Bulekov 
  Alexander D

[xen-unstable-smoke test] 152334: tolerable all pass - PUSHED

[osstest service owner]

flight 152334 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152334/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  81fd0d3ca4b2cd309403c6e8da662c325dd35750
baseline version:
 xen  a85f67b2658ed8032586b3a3e7cd78814d20aa4b

Last test of basis   152327  2020-07-31 15:02:14 Z0 days
Testing same since   152334  2020-07-31 20:01:29 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 
  Olaf Hering 
  Paul Durrant 
  Tim Deegan 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   a85f67b265..81fd0d3ca4  81fd0d3ca4b2cd309403c6e8da662c325dd35750 -> smoke

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Stefano Stabellini]

On Fri, 31 Jul 2020, Bertrand Marquis wrote:
> Sorry missed some points in my previous answer.
> 
> > On 30 Jul 2020, at 22:50, Julien Grall  wrote:
> > 
> > Hi Bertrand,
> > 
> > To avoid extra work on your side, I would recommend to wait a bit before 
> > sending a new version. It would be good to at least settle the conversation 
> > in v2 regarding the approach taken.
> > 
> > On 30/07/2020 11:24, Bertrand Marquis wrote:
> >> At the moment on Arm, a Linux guest running with KTPI enabled will
> >> cause the following error when a context switch happens in user mode:
> >> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0
> >> The error is caused by the virtual address for the runstate area
> >> registered by the guest only being accessible when the guest is running
> >> in kernel space when KPTI is enabled.
> >> To solve this issue, this patch is doing the translation from virtual
> >> address to physical address during the hypercall and mapping the
> >> required pages using vmap. This is removing the conversion from virtual
> >> to physical address during the context switch which is solving the
> >> problem with KPTI.
> > 
> > To echo what Jan said on the previous version, this is a change in a stable 
> > ABI and therefore may break existing guest. FAOD, I agree in principle with 
> > the idea. However, we want to explain why breaking the ABI is the *only* 
> > viable solution.
> > 
> > From my understanding, it is not possible to fix without an ABI breakage 
> > because the hypervisor doesn't know when the guest will switch back from 
> > userspace to kernel space. The risk is the information provided by the 
> > runstate wouldn't contain accurate information and could affect how the 
> > guest handle stolen time.
> > 
> > Additionally there are a few issues with the current interface:
> >   1) It is assuming the virtual address cannot be re-used by the userspace. 
> > Thanksfully Linux have a split address space. But this may change with KPTI 
> > in place.
> >   2) When update the page-tables, the guest has to go through an invalid 
> > mapping. So the translation may fail at any point.
> > 
> > IOW, the existing interface can lead to random memory corruption and 
> > inacurracy of the stolen time.
> > 
> >> This is done only on arm architecture, the behaviour on x86 is not
> >> modified by this patch and the address conversion is done as before
> >> during each context switch.
> >> This is introducing several limitations in comparison to the previous
> >> behaviour (on arm only):
> >> - if the guest is remapping the area at a different physical address Xen
> >> will continue to update the area at the previous physical address. As
> >> the area is in kernel space and usually defined as a global variable this
> >> is something which is believed not to happen. If this is required by a
> >> guest, it will have to call the hypercall with the new area (even if it
> >> is at the same virtual address).
> >> - the area needs to be mapped during the hypercall. For the same reasons
> >> as for the previous case, even if the area is registered for a different
> >> vcpu. It is believed that registering an area using a virtual address
> >> unmapped is not something done.
> > 
> > This is not clear whether the virtual address refer to the current vCPU or 
> > the vCPU you register the runstate for. From the past discussion, I think 
> > you refer to the former. It would be good to clarify.
> > 
> > Additionally, all the new restrictions should be documented in the public 
> > interface. So an OS developper can find the differences between the 
> > architectures.
> > 
> > To answer Jan's concern, we certainly don't know all the guest OSes 
> > existing, however we also need to balance the benefit for a large majority 
> > of the users.
> > 
> > From previous discussion, the current approach was deemed to be acceptable 
> > on Arm and, AFAICT, also x86 (see [1]).
> > 
> > TBH, I would rather see the approach to be common. For that, we would an 
> > agreement from Andrew and Jan in the approach here. Meanwhile, I think this 
> > is the best approach to address the concern from Arm users.
> > 
> >> inline functions in headers could not be used as the architecture
> >> domain.h is included before the global domain.h making it impossible
> >> to use the struct vcpu inside the architecture header.
> >> This should not have any performance impact as the hypercall is only
> >> called once per vcpu usually.
> >> Signed-off-by: Bertrand Marquis 
> >> ---
> >>   Changes in v2
> >> - use vmap to map the pages during the hypercall.
> >> - reintroduce initial copy during hypercall.
> >>   Changes in v3
> >> - Fix Coding style
> >> - Fix vaddr printing on arm32
> >> - use write_atomic to modify state_entry_time update bit (only
> >> in guest structure as the bit is not used inside Xen copy)
> >> ---
> >>  xen/arch/arm/domain.c| 161 ++-
> >>  xe

Re: [PATCH v2] xen/arm: Convert runstate address during hypcall

[Stefano Stabellini]

On Fri, 31 Jul 2020, Bertrand Marquis wrote:
> > On 31 Jul 2020, at 12:18, Jan Beulich  wrote:
> > 
> > On 31.07.2020 12:12, Julien Grall wrote:
> >> On 31/07/2020 07:39, Jan Beulich wrote:
> >>> We're fixing other issues without breaking the ABI. Where's the
> >>> problem of backporting the kernel side change (which I anticipate
> >>> to not be overly involved)?
> >> This means you can't take advantage of the runstage on existing Linux 
> >> without any modification.
> >> 
> >>> If the plan remains to be to make an ABI breaking change,
> >> 
> >> For a theoritical PoV, this is a ABI breakage. However, I fail to see 
> >> how the restrictions added would affect OSes at least on Arm.
> > 
> > "OSes" covering what? Just Linux?
> > 
> >> In particular, you can't change the VA -> PA on Arm without going 
> >> through an invalid mapping. So I wouldn't expect this to happen for the 
> >> runstate.
> >> 
> >> The only part that *may* be an issue is if the guest is registering the 
> >> runstate with an initially invalid VA. Although, I have yet to see that 
> >> in practice. Maybe you know?
> > 
> > I'm unaware of any such use, but this means close to nothing.
> > 
> >>> then I
> >>> think this will need an explicit vote.
> >> 
> >> I was under the impression that the two Arm maintainers (Stefano and I) 
> >> already agreed with the approach here. Therefore, given the ABI breakage 
> >> is only affecting Arm, why would we need a vote?
> > 
> > The problem here is of conceptual nature: You're planning to
> > make the behavior of a common hypercall diverge between
> > architectures, and in a retroactive fashion. Imo that's nothing
> > we should do even for new hypercalls, if _at all_ avoidable. If
> > we allow this here, we'll have a precedent that people later
> > may (and based on my experience will, sooner or later) reference,
> > to get their own change justified.

Please let's avoid "slippery slope" arguments
(https://en.wikipedia.org/wiki/Slippery_slope)

We shouldn't consider this instance as the first in a long series of bad
decisions on hypercall compatibility. Each new case, if there will be
any, will have to be considered based on its own merits. Also, let's
keep in mind that there have been no other cases in the last 8 years. (I
would like to repeat my support for hypercall ABI compatibility.)


I would also kindly ask not to put the discussion on a "conceptual"
level: there is no way to fix all guests and also keep compatibility.
>From a conceptual point of view, it is already game over :-)


> After a discussion with Jan, he is proposing to have a guest config setting to
> turn on or off the translation of the address during the hypercall and add a
> global Xen command line parameter to set the global default behaviour. 
> With this was done on arm could be done on x86 and the current behaviour
> would be kept by default but possible to modify by configuration.
> 
> @Jan: please correct me if i said something wrong
> @others: what is your view on this solution ?

Having options to turn on or off the new behavior could be good-to-have
if we find a guest that actually requires the old behavior. Today we
don't know of any such cases. We have strong reasons to believe that
there aren't any on ARM (see Julien's explanation in regards to the
temporary invalid mappings.) In fact, it is one of the factors that led
us to think this patch is the right approach.

That said, I am also OK with adding such a parameter now, but we need to
choose the default value carefully.


We need the new behavior as default on ARM because we need the fix to
work for all guests. I don't think we want to explain how you always
need to set config_foobar otherwise things don't work. It has to work
out of the box.

It would be nice if we had the same default on x86 too, although I
understand if Jan and Andrew don't want to make the same change on x86,
at least initially.

[linux-linus test] 152313: regressions - FAIL

[osstest service owner]

flight 152313 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152313/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-freebsd10-i386  7 xen-boot   fail REGR. vs. 152287
 test-amd64-amd64-amd64-pvgrub 17 guest-localmigrate/x10  fail REGR. vs. 152287

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152287
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152287
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152287
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 152287
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 152287
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152287
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152287
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 152287
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152287
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 linux417385c47ef7ee0d4f48f63f70cca6c1ed6355f4
baseline version:
 linux6ba1b005ffc388c2aeaddae20da29e4810dea298

Last test of basis   152287  2020-07-29 17:11:28 Z2 days
Failing since152303  2020-07-30 11:09:02 Z1 days2 attempts
Testing same since   152313  2020-07-31 02:28:59 Z0 days1 attempts

-

[xen-unstable-smoke test] 152327: tolerable all pass - PUSHED

[osstest service owner]

flight 152327 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152327/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  a85f67b2658ed8032586b3a3e7cd78814d20aa4b
baseline version:
 xen  98bed5de1de3352c63cfe29a00f17e8d9ce72689

Last test of basis   152310  2020-07-30 22:00:29 Z0 days
Testing same since   152327  2020-07-31 15:02:14 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Ian Jackson 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   98bed5de1d..a85f67b265  a85f67b2658ed8032586b3a3e7cd78814d20aa4b -> smoke

RE: [PATCH v3 1/2] x86/hvm: set 'ipat' in EPT for special pages

[Durrant, Paul]

> -Original Message-
> From: Paul Durrant 
> Sent: 31 July 2020 15:26
> To: xen-devel@lists.xenproject.org
> Cc: Durrant, Paul ; Jan Beulich ; 
> Andrew Cooper
> ; Wei Liu ; Roger Pau Monné 
> 
> Subject: [EXTERNAL] [PATCH v3 1/2] x86/hvm: set 'ipat' in EPT for special 
> pages
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> From: Paul Durrant 
> 
> All non-MMIO ranges (i.e those not mapping real device MMIO regions) that
> map valid MFNs are normally marked MTRR_TYPE_WRBACK and 'ipat' is set. Hence
> when PV drivers running in a guest populate the BAR space of the Xen Platform
> PCI Device with pages such as the Shared Info page or Grant Table pages,
> accesses to these pages will be cachable.
> 
> However, should IOMMU mappings be enabled be enabled for the guest then these
> accesses become uncachable. This has a substantial negative effect on I/O
> throughput of PV devices. Arguably PV drivers should bot be using BAR space to
> host the Shared Info and Grant Table pages but it is currently commonplace for
> them to do this and so this problem needs mitigation. Hence this patch makes
> sure the 'ipat' bit is set for any special page regardless of where in GFN
> space it is mapped.
> 
> NOTE: Clearly this mitigation only applies to Intel EPT. It is not obvious
>   that there is any similar mitigation possible for AMD NPT. Downstreams
>   such as Citrix XenServer have been carrying a patch similar to this for
>   several releases though.
> 
> Signed-off-by: Paul Durrant 

This is missing a hunk. I'll send v4.

  Paul

> ---
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Wei Liu 
> Cc: "Roger Pau Monné" 
> 
> v3:
>  - dropping Jan's R-b
>  - cope with order > 0
> ---
>  xen/arch/x86/hvm/mtrr.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 511c3be1c8..26721f6ee7 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -836,6 +836,17 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>  return MTRR_TYPE_WRBACK;
>  }
> 
> +for ( i = 0; i < (1ul << order); i++ )
> +{
> +if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
> +{
> +if ( order )
> +return -1;
> +*ipat = 1;
> +return MTRR_TYPE_WRBACK;
> +}
> +}
> +
>  gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
>  if ( gmtrr_mtype >= 0 )
>  {
> --
> 2.20.1

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Durrant, Paul]

> -Original Message-
> From: Jan Beulich 
> Sent: 31 July 2020 14:41
> To: p...@xen.org
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul ; 
> 'Andrew Cooper'
> ; 'Wei Liu' ; 'Roger Pau Monné' 
> 
> Subject: RE: [EXTERNAL] [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check 
> in epte_get_entry_emt()
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 31.07.2020 15:07, Paul Durrant wrote:
> >> -Original Message-
> >> From: Jan Beulich 
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant 
> >> Cc: xen-devel@lists.xenproject.org; Paul Durrant ; 
> >> Andrew Cooper
> >> ; Wei Liu ; Roger Pau Monné 
> >> 
> >> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> >> epte_get_entry_emt()
> >>
> >> On 31.07.2020 14:39, Paul Durrant wrote:
> >>> From: Paul Durrant 
> >>>
> >>> Re-factor the code to take advantage of the fact that the APIC access 
> >>> page is
> >>> a 'special' page.
> >>
> >> Hmm, that's going quite as far as I was thinking to go: In
> >> particular, you leave in place the set_mmio_p2m_entry() use
> >> in vmx_alloc_vlapic_mapping(). With that replaced, the
> >> re-ordering in epte_get_entry_emt() that you do shouldn't
> >> be necessary; you'd simple drop the checking of the
> >> specific MFN.
> >
> > Ok, it still needs to go in the p2m though so are you suggesting
> > just calling p2m_set_entry() directly?
> 
> Yes, if this works. The main question really is whether there are
> any hidden assumptions elsewhere that this page gets mapped as an
> MMIO one.
> 
> >>> --- a/xen/arch/x86/hvm/mtrr.c
> >>> +++ b/xen/arch/x86/hvm/mtrr.c
> >>> @@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned 
> >>> long gfn, mfn_t mfn,
> >>>  return -1;
> >>>  }
> >>>
> >>> -if ( direct_mmio )
> >>> -{
> >>> -if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> 
> >>> order )
> >>> -return MTRR_TYPE_UNCACHABLE;
> >>> -if ( order )
> >>> -return -1;
> >>
> >> ... this part of the logic wants retaining, I think, i.e.
> >> reporting back to the guest that the mapping needs splitting.
> >> I'm afraid I have to withdraw my R-b on patch 1 for this
> >> reason, as the check needs to be added there already.
> >
> > To be clear... You mean I need the:
> >
> > if ( order )
> >   return -1;
> >
> > there?
> 
> Not just - first of all you need to check whether the requested
> range overlaps a special page.

Ok. I'll add that.

  Paul


> 
> Jan

Re: kernel-doc and xen.git

[Stefano Stabellini]

On Fri, 31 Jul 2020, George Dunlap wrote:
> > On Jul 31, 2020, at 12:29 PM, Jan Beulich  wrote:
> > 
> > On 30.07.2020 03:27, Stefano Stabellini wrote:
> >> Hi all,
> >> 
> >> I would like to ask for your feedback on the adoption of the kernel-doc
> >> format for in-code comments.
> >> 
> >> In the FuSa SIG we have started looking into FuSa documents for Xen. One
> >> of the things we are investigating are ways to link these documents to
> >> in-code comments in xen.git and vice versa.
> >> 
> >> In this context, Andrew Cooper suggested to have a look at "kernel-doc"
> >> [1] during one of the virtual beer sessions at the last Xen Summit.
> >> 
> >> I did give a look at kernel-doc and it is very promising. kernel-doc is
> >> a script that can generate nice rst text documents from in-code
> >> comments. (The generated rst files can then be used as input for sphinx
> >> to generate html docs.) The comment syntax [2] is simple and similar to
> >> Doxygen:
> >> 
> >>/**
> >> * function_name() - Brief description of function.
> >> * @arg1: Describe the first argument.
> >> * @arg2: Describe the second argument.
> >> *One can provide multiple line descriptions
> >> *for arguments.
> >> */
> >> 
> >> kernel-doc is actually better than Doxygen because it is a much simpler
> >> tool, one we could customize to our needs and with predictable output.
> >> Specifically, we could add the tagging, numbering, and referencing
> >> required by FuSa requirement documents.
> >> 
> >> I would like your feedback on whether it would be good to start
> >> converting xen.git in-code comments to the kernel-doc format so that
> >> proper documents can be generated out of them. One day we could import
> >> kernel-doc into xen.git/scripts and use it to generate a set of html
> >> documents via sphinx.
> > 
> > How far is this intended to go? The example is description of a
> > function's parameters, which is definitely fine (albeit I wonder
> > if there's a hidden implication then that _all_ functions
> > whatsoever are supposed to gain such comments). But the text just
> > says much more generally "in-code comments", which could mean all
> > of them. I'd consider the latter as likely going too far.
> 
> I took him to mean comments in the code at the moment, which describe some 
> interface, but aren’t in kernel-doc format.  Naturally we wouldn’t want *all* 
> comments to be stuffed into a document somewhere.

+1

Re: Ping: [PATCH v2] x86emul: avoid assembler warning about .type not taking effect in test harness

[Andrew Cooper]

On 31/07/2020 15:54, Jan Beulich wrote:
> On 14.07.2020 10:06, Jan Beulich wrote:
>> gcc re-orders top level blocks by default when optimizing. This
>> re-ordering results in all our .type directives to get emitted to the
>> assembly file first, followed by gcc's. The assembler warns about
>> attempts to change the type of a symbol when it was already set (and
>> when there's no intervening setting to "notype").
>>
>> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooper 

[OSSTEST PATCH] Disable mercurial support

[Ian Jackson]

This is in order that we can substantially simplify forthcoming
database changes.  If mercurial support were still desired, the right
thing to do would be to rework it now along the lines of this request.
But we haven't used it for some years.

It could be reenabled later, if this work were done then.  (Of course
there might be other bitrot already that we don't know about.)

CC: committ...@xenproject.org
Signed-off-by: Ian Jackson 
---
 Osstest/TestSupport.pm |  5 +
 sg-report-flight   | 11 ++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 7eeac49f..faac106f 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1661,6 +1661,11 @@ sub build_url_vcs ($) {
$tree = git_massage_url($tree);
 }
 
+if ($vcs eq 'hg') {
+   die "mercurial support has rottted";
+   # to reinstate, git grep for "mercurial" and fix everything
+}
+
 return ($tree, $vcs);
 }
 
diff --git a/sg-report-flight b/sg-report-flight
index 831917a9..49f7ba6a 100755
--- a/sg-report-flight
+++ b/sg-report-flight
@@ -299,7 +299,16 @@ END
 last;
 }
 my ($wronginfo) = grep {
-$_->[1]{val} !~ m/^(?: .*: )? $v /x;
+$_->[1]{val} ne $v;
+# Was once   $_->[1]{val} !~ m/^(?: .*: )? $v /x;
+   # to support stripping (local) changeset numbers from
+   # mercurial revisions in the val column.  But this
+   # does not work with our index query strategy.  To
+   # reinstate mercurial support, it will be necessary to
+   # either make the index query more complicated (eg an
+   # index on a substring of val) or to arrange for all
+   # the code to not ever store these revision counts in
+   # the db.  The latter is probably more correct.
 } @revisions;
 
 if (defined $wronginfo) {
-- 
2.20.1

Re: [OSSTEST PATCH v2 08/41] sg-report-flight: Ask the db for flights of interest

[Ian Jackson]

George Dunlap writes ("Re: [OSSTEST PATCH v2 08/41] sg-report-flight: Ask the 
db for flights of interest"):
> > On Jul 31, 2020, at 12:37 PM, Ian Jackson  wrote:
> > Specifically, we narrow the initial query to flights which have at
> > least some job with the built_revision_foo we are looking for.
> > 
> > This condition is strictly broader than that implemented inside the
> > flight search loop, so there is no functional change.
> 
> Assuming this is true, that job / runvar is filtered after extracting this 
> information, then...
...
> …I agree that this shoud introduce no other changes.
> 
> Reviewed-by: George Dunlap 

Thanks.

Just to convince myself, I ran through the argument based on the perl
code.  I found a lacuna.

1. The job of findaflight is to find a flight, and it doesn't have
   significant side effects - just a return value.

2. If it returns a flight from the loop, $whynot must have been
   undef.  $whynot is never unset.

Consider some tree in %{ $specver{$thisthat} }.

3. If @revisions is 0 for that tree, $whynot is set.  So one of the
   two queries $revisionsq or $revisionsosstestq must have returned
   some rows.

4. Furthermore, none of those rows must have passed the $wronginfo
   grep.  If they had, $whynot would have been set.  Any row
   whose val doesn't contain a colon, and which doesn't end up
   in $wronginfo, had a val equal to the requested specver.

5. Colons in this field appear only in mercurial revisions.  These are
   now obsoelete - we have no mercurial trees.  A consequence of this
   commit is actually that we should explicitly abolish mercurial
   support, at least pending a change to osstest to arrange for the
   val column to contain only the hash part and not the number part.

6. Together, these conditons means that if $whynot wasn't set,
   there must have been some row whose val matched the specver.

7. Both the $revisionsq and $revisionsosstestq queries take a flight
   bound variable condition.  This is bound by a value that came out
   of @binfos.  @binfos is made from %binfos, where the flight number
   is the key.  %binfos is populated by the @binfos_todo loop, where
   it gets the flight number from a @binfos_todos entry - but it
   filters them for $bflight == $tflight.

8. So some row must have matched the flight, and the specver, and
   of course the name.  This is precisely the new condition.

I think this means I should put a commit earlier in this series which
disables mercurial support until the colon version situation is
rationalised.

Ian.

Re: Ping: [PATCH 3/5] x86/PV: drop a few misleading paging_mode_refcounts() checks

[Andrew Cooper]

On 31/07/2020 15:58, Jan Beulich wrote:
> On 15.07.2020 11:59, Jan Beulich wrote:
>> The filling and cleaning up of v->arch.guest_table in new_guest_cr3()
>> was apparently inconsistent so far: There was a type ref acquired
>> unconditionally for the new top level page table, but the dropping of
>> the old type ref was conditional upon !paging_mode_refcounts(). Mirror
>> this also to arch_set_info_guest().
>>
>> Also move new_guest_cr3()'s #ifdef to around the function - both callers
>> now get built only when CONFIG_PV, i.e. no need to retain a stub.
>>
>> Signed-off-by: Jan Beulich 
> While I've got an ack from Tim, I think I need either an ack from
> Andrew or someone's R-b in order to commit this.

Acked-by: Andrew Cooper 

[ovmf test] 152315: all pass - PUSHED

[osstest service owner]

flight 152315 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152315/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 7f79b736b0a57da71d87c987357db0227cd16ac6
baseline version:
 ovmf e848b58d7c85293cd4121287abcea2d22a4f0620

Last test of basis   152277  2020-07-29 04:16:17 Z2 days
Testing same since   152315  2020-07-31 03:12:00 Z0 days1 attempts


People who touched revisions under test:
  Shenglei Zhang 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   e848b58d7c..7f79b736b0  7f79b736b0a57da71d87c987357db0227cd16ac6 -> 
xen-tested-master

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Julien Grall]

Hi Bertrand,

On 31/07/2020 14:09, Bertrand Marquis wrote:




On 31 Jul 2020, at 14:19, Jan Beulich  wrote:

On 30.07.2020 22:50, Julien Grall wrote:

On 30/07/2020 11:24, Bertrand Marquis wrote:

At the moment on Arm, a Linux guest running with KTPI enabled will
cause the following error when a context switch happens in user mode:
(XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0

The error is caused by the virtual address for the runstate area
registered by the guest only being accessible when the guest is running
in kernel space when KPTI is enabled.

To solve this issue, this patch is doing the translation from virtual
address to physical address during the hypercall and mapping the
required pages using vmap. This is removing the conversion from virtual
to physical address during the context switch which is solving the
problem with KPTI.


To echo what Jan said on the previous version, this is a change in a
stable ABI and therefore may break existing guest. FAOD, I agree in
principle with the idea. However, we want to explain why breaking the
ABI is the *only* viable solution.

 From my understanding, it is not possible to fix without an ABI
breakage because the hypervisor doesn't know when the guest will switch
back from userspace to kernel space.


And there's also no way to know on Arm, by e.g. enabling a suitable
intercept?


There is no easy way to do it. You might be able to route all EL0 
exceptions to EL2 using HCR_EL2.TGE, but this is basically disable EL1 
(kernel space). The amount of work required and the overhead is likely 
not worth it.




An intercept would mean that Xen gets a notice whenever a guest is switching
from kernel mode to user mode.
There is nothing in this process which could be intercepted by Xen, appart from
maybe trapping all access to MMU registers which would be very complex and
slow.


I agree. Although, even if it wasn't slow, there is no guarantee that 
any of those registers would be accessed during the switch.


You could implement a "dumb" KPTI by just removing the mappings from the 
page-tables.


Cheers,

--
Julien Grall

Ping: [PATCH 3/5] x86/PV: drop a few misleading paging_mode_refcounts() checks

[Jan Beulich]

On 15.07.2020 11:59, Jan Beulich wrote:
> The filling and cleaning up of v->arch.guest_table in new_guest_cr3()
> was apparently inconsistent so far: There was a type ref acquired
> unconditionally for the new top level page table, but the dropping of
> the old type ref was conditional upon !paging_mode_refcounts(). Mirror
> this also to arch_set_info_guest().
> 
> Also move new_guest_cr3()'s #ifdef to around the function - both callers
> now get built only when CONFIG_PV, i.e. no need to retain a stub.
> 
> Signed-off-by: Jan Beulich 

While I've got an ack from Tim, I think I need either an ack from
Andrew or someone's R-b in order to commit this.

Thanks, Jan

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1122,8 +1122,6 @@ int arch_set_info_guest(
>  
>  if ( !cr3_page )
>  rc = -EINVAL;
> -else if ( paging_mode_refcounts(d) )
> -/* nothing */;
>  else if ( cr3_page == v->arch.old_guest_table )
>  {
>  v->arch.old_guest_table = NULL;
> @@ -1144,8 +1142,7 @@ int arch_set_info_guest(
>  case -ERESTART:
>  break;
>  case 0:
> -if ( !compat && !VM_ASSIST(d, m2p_strict) &&
> - !paging_mode_refcounts(d) )
> +if ( !compat && !VM_ASSIST(d, m2p_strict) )
>  fill_ro_mpt(cr3_mfn);
>  break;
>  default:
> @@ -1166,7 +1163,7 @@ int arch_set_info_guest(
>  
>  if ( !cr3_page )
>  rc = -EINVAL;
> -else if ( !paging_mode_refcounts(d) )
> +else
>  {
>  rc = get_page_type_preemptible(cr3_page, 
> PGT_root_page_table);
>  switch ( rc )
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3149,9 +3149,9 @@ int vcpu_destroy_pagetables(struct vcpu
>  return rc;
>  }
>  
> +#ifdef CONFIG_PV
>  int new_guest_cr3(mfn_t mfn)
>  {
> -#ifdef CONFIG_PV
>  struct vcpu *curr = current;
>  struct domain *d = curr->domain;
>  int rc;
> @@ -3220,7 +3220,7 @@ int new_guest_cr3(mfn_t mfn)
>  
>  pv_destroy_ldt(curr); /* Unconditional TLB flush later. */
>  
> -if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
> +if ( !VM_ASSIST(d, m2p_strict) )
>  fill_ro_mpt(mfn);
>  curr->arch.guest_table = pagetable_from_mfn(mfn);
>  update_cr3(curr);
> @@ -3231,30 +3231,24 @@ int new_guest_cr3(mfn_t mfn)
>  {
>  struct page_info *page = mfn_to_page(old_base_mfn);
>  
> -if ( paging_mode_refcounts(d) )
> -put_page(page);
> -else
> -switch ( rc = put_page_and_type_preemptible(page) )
> -{
> -case -EINTR:
> -case -ERESTART:
> -curr->arch.old_guest_ptpg = NULL;
> -curr->arch.old_guest_table = page;
> -curr->arch.old_guest_table_partial = (rc == -ERESTART);
> -rc = -ERESTART;
> -break;
> -default:
> -BUG_ON(rc);
> -break;
> -}
> +switch ( rc = put_page_and_type_preemptible(page) )
> +{
> +case -EINTR:
> +case -ERESTART:
> +curr->arch.old_guest_ptpg = NULL;
> +curr->arch.old_guest_table = page;
> +curr->arch.old_guest_table_partial = (rc == -ERESTART);
> +rc = -ERESTART;
> +break;
> +default:
> +BUG_ON(rc);
> +break;
> +}
>  }
>  
>  return rc;
> -#else
> -ASSERT_UNREACHABLE();
> -return -EINVAL;
> -#endif
>  }
> +#endif
>  
>  #ifdef CONFIG_PV
>  static int vcpumask_to_pcpumask(
> 
> 

Re: [OSSTEST PATCH v2 07/41] schema: Provide indices for sg-report-flight

[Ian Jackson]

George Dunlap writes ("Re: [OSSTEST PATCH v2 07/41] schema: Provide indices for 
sg-report-flight"):
> 
> 
> > On Jul 31, 2020, at 12:37 PM, Ian Jackson  wrote:
> > 
> > These indexes allow very fast lookup of "relevant" flights eg when
> > trying to justify failures.
> > 
> > In my ad-hoc test case, these indices (along with the subsequent
> > changes to sg-report-flight and Executive.pm, reduce the runtime of
> > sg-report-flight from 2-3ks (unacceptably long!) to as little as
> > 5-7s seconds - a speedup of about 500x.
> > 
> > (Getting the database snapshot may take a while first, but deploying
> > this code should help with that too by reducing long-running
> > transactions.  Quoted perf timings are from snapshot acquisition.)
> > 
> > Without these new indexes there may be a performance change from the
> > query changes.  I haven't benchmarked this so I am setting the schema
> > updates to be Preparatory/Needed (ie, "Schema first" as
> > schema/README.updates has it), to say that the index should be created
> > before the new code is deployed.
> > 
> > Testing: I have tested this series by creating experimental indices
> > "trial_..." in the actual production instance.  (Transactional DDL was
> > very helpful with this.)  I have verified with \d that schema update
> > instructions in this commit generate indexes which are equivalent to
> > the trial indices.
> > 
> > Deployment: AFter these schema updates are applied, the trial indices
> > are redundant duplicates and should be deleted.
...
> 
> I have no idea if building an index on a LIKE is a good idea or not, but it 
> certainly seems to be useful, so:
> 
> Reviewed-by: George Dunlap 

Thanks.

This is a thing called a "partial index", where the index only covers
some subset of the rows.  The subset is determined a condition on the
row contents.

Such an index can be a lot smaller than an index on the whole table
and also avoids slowing down updates that don't match the index
condition.

The idea is that when the query contains a condition that matches the
index condition, the query planner can use this small on-topic index
instead of wading through something large and irrelevant.

The query planner is not always very bright about what conditions are
subsets of what other conditions, and it runs without seeing the
contents of bind variables.  So with LIKE, for example, it's generally
necessary to precisely replicate the index condition in the queries.
That's why some of the queries in this series have things like this:

  AND r$ri.name LIKE 'built\_revision\_%'
  AND r$ri.name = ?

where the Perl code passes in 'built_revison_something'.

I hope this explanation was interesting :-).

Ian.

Ping: [PATCH] x86/CPUID: move some static masks into .init

[Jan Beulich]

On 15.07.2020 09:45, Jan Beulich wrote:
> Except for hvm_shadow_max_featuremask and deep_features they're
> referenced by __init functions only.
> 
> Signed-off-by: Jan Beulich 
> 
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -16,12 +16,15 @@
>  const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>  const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
>  
> -static const uint32_t pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
> +static const uint32_t __initconst pv_max_featuremask[] = 
> INIT_PV_MAX_FEATURES;
>  static const uint32_t hvm_shadow_max_featuremask[] = 
> INIT_HVM_SHADOW_MAX_FEATURES;
> -static const uint32_t hvm_hap_max_featuremask[] = INIT_HVM_HAP_MAX_FEATURES;
> -static const uint32_t pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
> -static const uint32_t hvm_shadow_def_featuremask[] = 
> INIT_HVM_SHADOW_DEF_FEATURES;
> -static const uint32_t hvm_hap_def_featuremask[] = INIT_HVM_HAP_DEF_FEATURES;
> +static const uint32_t __initconst hvm_hap_max_featuremask[] =
> +INIT_HVM_HAP_MAX_FEATURES;
> +static const uint32_t __initconst pv_def_featuremask[] = 
> INIT_PV_DEF_FEATURES;
> +static const uint32_t __initconst hvm_shadow_def_featuremask[] =
> +INIT_HVM_SHADOW_DEF_FEATURES;
> +static const uint32_t __initconst hvm_hap_def_featuremask[] =
> +INIT_HVM_HAP_DEF_FEATURES;
>  static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
>  
>  static int __init parse_xen_cpuid(const char *s)
> 

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Andrew Cooper]

On 31/07/2020 15:44, Jan Beulich wrote:
> On 28.07.2020 13:37, Andrew Cooper wrote:
>> @@ -1026,19 +1047,6 @@ static int acquire_resource(
>>  if ( xmar.pad != 0 )
>>  return -EINVAL;
>>  
>> -if ( guest_handle_is_null(xmar.frame_list) )
>> -{
>> -if ( xmar.nr_frames )
>> -return -EINVAL;
>> -
>> -xmar.nr_frames = ARRAY_SIZE(mfn_list);
>> -
>> -if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
>> -return -EFAULT;
>> -
>> -return 0;
>> -}
>> -
>>  if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
>>  return -E2BIG;
> While arguably minor, the error code in the null-handle case
> would imo better be the same, no matter how big xmar.nr_frames
> is.

This clause doesn't survive the fixes to batching.

Given how broken this infrastructure is, I'm not concerned with
transient differences in error codes for users which will ultimately
fail anyway.

~Andrew

Ping: [PATCH v2] x86emul: avoid assembler warning about .type not taking effect in test harness

[Jan Beulich]

On 14.07.2020 10:06, Jan Beulich wrote:
> gcc re-orders top level blocks by default when optimizing. This
> re-ordering results in all our .type directives to get emitted to the
> assembly file first, followed by gcc's. The assembler warns about
> attempts to change the type of a symbol when it was already set (and
> when there's no intervening setting to "notype").
> 
> Signed-off-by: Jan Beulich 
> ---
> v2: Refine description to no longer claim a gcc change to be the reason.
> 
> --- a/tools/tests/x86_emulator/Makefile
> +++ b/tools/tests/x86_emulator/Makefile
> @@ -295,4 +295,9 @@ x86-emulate.o cpuid.o test_x86_emulator.
>  x86-emulate.o: x86_emulate/x86_emulate.c
>  x86-emulate.o: HOSTCFLAGS += -D__XEN_TOOLS__
>  
> +# In order for our custom .type assembler directives to reliably land after
> +# gcc's, we need to keep it from re-ordering top-level constructs.
> +$(call cc-option-add,HOSTCFLAGS-toplevel,HOSTCC,-fno-toplevel-reorder)
> +test_x86_emulator.o: HOSTCFLAGS += $(HOSTCFLAGS-toplevel)
> +
>  test_x86_emulator.o: $(addsuffix .h,$(TESTCASES)) $(addsuffix 
> -opmask.h,$(OPMASK))
> 

Re: [PATCH v4 0/2] epte_get_entry_emt() modifications

[Jan Beulich]

On 31.07.2020 16:41, Paul Durrant wrote:
> From: Paul Durrant 
> 
> Paul Durrant (2):
>   x86/hvm: set 'ipat' in EPT for special pages
>   x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

Reviewed-by: Jan Beulich 

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Jan Beulich]

On 28.07.2020 13:37, Andrew Cooper wrote:
> @@ -1026,19 +1047,6 @@ static int acquire_resource(
>  if ( xmar.pad != 0 )
>  return -EINVAL;
>  
> -if ( guest_handle_is_null(xmar.frame_list) )
> -{
> -if ( xmar.nr_frames )
> -return -EINVAL;
> -
> -xmar.nr_frames = ARRAY_SIZE(mfn_list);
> -
> -if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> -return -EFAULT;
> -
> -return 0;
> -}
> -
>  if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
>  return -E2BIG;

While arguably minor, the error code in the null-handle case
would imo better be the same, no matter how big xmar.nr_frames
is.

Jan

[PATCH v4 0/2] epte_get_entry_emt() modifications

[Paul Durrant]

From: Paul Durrant 

Paul Durrant (2):
  x86/hvm: set 'ipat' in EPT for special pages
  x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

 xen/arch/x86/hvm/mtrr.c | 27 ---
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH v4 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

From: Paul Durrant 

Re-factor the code to take advantage of the fact that the APIC access page is
a 'special' page. The VMX code is left alone and hence the APIC access page is
still inserted into the P2M with type p2m_mmio_direct. This is left alone as it
is not obvious there is another suitable type to use, and the necessary
re-ordering in epte_get_entry_emt() is straightforward.

Suggested-by: Jan Beulich 
Signed-off-by: Paul Durrant 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 

v2:
 - New in v2

v3:
 - Re-base
 - Expand commit comment
---
 xen/arch/x86/hvm/mtrr.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 2bd64e8025..fb051d59c3 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -815,23 +815,13 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return -1;
 }
 
-if ( direct_mmio )
-{
-if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
-return MTRR_TYPE_UNCACHABLE;
-if ( order )
-return -1;
-*ipat = 1;
-return MTRR_TYPE_WRBACK;
-}
-
 if ( !mfn_valid(mfn) )
 {
 *ipat = 1;
 return MTRR_TYPE_UNCACHABLE;
 }
 
-if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
+if ( !direct_mmio && !is_iommu_enabled(d) && !cache_flush_permitted(d) )
 {
 *ipat = 1;
 return MTRR_TYPE_WRBACK;
@@ -848,6 +838,9 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
mfn_t mfn,
 }
 }
 
+if ( direct_mmio )
+return MTRR_TYPE_UNCACHABLE;
+
 gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
 if ( gmtrr_mtype >= 0 )
 {
-- 
2.20.1

[PATCH v4 1/2] x86/hvm: set 'ipat' in EPT for special pages

[Paul Durrant]

From: Paul Durrant 

All non-MMIO ranges (i.e those not mapping real device MMIO regions) that
map valid MFNs are normally marked MTRR_TYPE_WRBACK and 'ipat' is set. Hence
when PV drivers running in a guest populate the BAR space of the Xen Platform
PCI Device with pages such as the Shared Info page or Grant Table pages,
accesses to these pages will be cachable.

However, should IOMMU mappings be enabled be enabled for the guest then these
accesses become uncachable. This has a substantial negative effect on I/O
throughput of PV devices. Arguably PV drivers should bot be using BAR space to
host the Shared Info and Grant Table pages but it is currently commonplace for
them to do this and so this problem needs mitigation. Hence this patch makes
sure the 'ipat' bit is set for any special page regardless of where in GFN
space it is mapped.

NOTE: Clearly this mitigation only applies to Intel EPT. It is not obvious
  that there is any similar mitigation possible for AMD NPT. Downstreams
  such as Citrix XenServer have been carrying a patch similar to this for
  several releases though.

Signed-off-by: Paul Durrant 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 

v3:
 - Dropping Jan's R-b
 - Cope with order > 0

v4:
 - Add missing hunk
---
 xen/arch/x86/hvm/mtrr.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 511c3be1c8..2bd64e8025 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -794,6 +794,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
mfn_t mfn,
 {
 int gmtrr_mtype, hmtrr_mtype;
 struct vcpu *v = current;
+unsigned long i;
 
 *ipat = 0;
 
@@ -836,6 +837,17 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return MTRR_TYPE_WRBACK;
 }
 
+for ( i = 0; i < (1ul << order); i++ )
+{
+if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
+{
+if ( order )
+return -1;
+*ipat = 1;
+return MTRR_TYPE_WRBACK;
+}
+}
+
 gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
 if ( gmtrr_mtype >= 0 )
 {
-- 
2.20.1

Re: [PATCH v2] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis]

> On 31 Jul 2020, at 12:18, Jan Beulich  wrote:
> 
> On 31.07.2020 12:12, Julien Grall wrote:
>> On 31/07/2020 07:39, Jan Beulich wrote:
>>> We're fixing other issues without breaking the ABI. Where's the
>>> problem of backporting the kernel side change (which I anticipate
>>> to not be overly involved)?
>> This means you can't take advantage of the runstage on existing Linux 
>> without any modification.
>> 
>>> If the plan remains to be to make an ABI breaking change,
>> 
>> For a theoritical PoV, this is a ABI breakage. However, I fail to see 
>> how the restrictions added would affect OSes at least on Arm.
> 
> "OSes" covering what? Just Linux?
> 
>> In particular, you can't change the VA -> PA on Arm without going 
>> through an invalid mapping. So I wouldn't expect this to happen for the 
>> runstate.
>> 
>> The only part that *may* be an issue is if the guest is registering the 
>> runstate with an initially invalid VA. Although, I have yet to see that 
>> in practice. Maybe you know?
> 
> I'm unaware of any such use, but this means close to nothing.
> 
>>> then I
>>> think this will need an explicit vote.
>> 
>> I was under the impression that the two Arm maintainers (Stefano and I) 
>> already agreed with the approach here. Therefore, given the ABI breakage 
>> is only affecting Arm, why would we need a vote?
> 
> The problem here is of conceptual nature: You're planning to
> make the behavior of a common hypercall diverge between
> architectures, and in a retroactive fashion. Imo that's nothing
> we should do even for new hypercalls, if _at all_ avoidable. If
> we allow this here, we'll have a precedent that people later
> may (and based on my experience will, sooner or later) reference,
> to get their own change justified.

After a discussion with Jan, he is proposing to have a guest config setting to
turn on or off the translation of the address during the hypercall and add a
global Xen command line parameter to set the global default behaviour. 
With this was done on arm could be done on x86 and the current behaviour
would be kept by default but possible to modify by configuration.

@Jan: please correct me if i said something wrong
@others: what is your view on this solution ?

Bertrand

Re: [PATCH 4/5] xen/memory: Fix acquire_resource size semantics

[Jan Beulich]

On 30.07.2020 21:46, Andrew Cooper wrote:
> On 30/07/2020 09:31, Paul Durrant wrote:
>>> From: Xen-devel  On Behalf Of 
>>> Andrew Cooper
>>> Sent: 28 July 2020 12:37
>>>
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4013,6 +4013,25 @@ static int gnttab_get_shared_frame_mfn(struct domain 
>>> *d,
>>>  return 0;
>>>  }
>>>
>>> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id)
>>> +{
>>> +unsigned int nr = 0;
>>> +
>>> +/* Don't need the grant lock.  This limit is fixed at domain create 
>>> time. */
>>> +switch ( id )
>>> +{
>>> +case XENMEM_resource_grant_table_id_shared:
>>> +nr = d->grant_table->max_grant_frames;
>>> +break;
>>> +
>>> +case XENMEM_resource_grant_table_id_status:
>>> +nr = grant_to_status_frames(d->grant_table->max_grant_frames);
>> Two uses of d->grant_table, so perhaps define a stack variable for it?
> 
> Can do.
> 
>>  Also, should you not make sure 0 is returned in the case of a v1 table?
> 
> This was the case specifically discussed in the commit message, but
> perhaps it needs expanding.
> 
> Doing so would be buggy.
> 
> Some utility is going to query the resource size, and then try to map it
> (if it doesn't blindly know the size and/or subset it cares about already).
> 
> In between these two hypercalls from the utility, the guest can do a
> v1=>v2 or v2=>v1 switch and make the resource spontaneously appear or
> disappear.
> 
> The only case where we can know for certain whether the resource is
> available is when we're in the map hypercall.  Therefore, userspace has
> to be able to get to the map call if there is potentially a resource
> available.
> 
> The semantics of the size call are really "this resource might exist,
> and if it does, this is how large it is".

With you deriving from d->grant_table->max_grant_frames, this approach
would imply that by obtaining a mapping the grant tables will get
grown to their permitted maximum, no matter whether as much is actually
needed by the guest. If this is indeed the intention, then we could as
well set up maximum grant structures right at domain creation. Not
something I would favor, but anyway...

Jan

[PATCH v3 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

From: Paul Durrant 

Re-factor the code to take advantage of the fact that the APIC access page is
a 'special' page. The VMX code is left alone and hence the APIC access page is
still inserted into the P2M with type p2m_mmio_direct. This is left alone as it
is not obvious there is another suitable type to use, and the necessary
re-ordering in epte_get_entry_emt() is straightforward.

Suggested-by: Jan Beulich 
Signed-off-by: Paul Durrant 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 

v2:
 - New in v2

v3:
 - Re-base
 - Expand commit comment
---
 xen/arch/x86/hvm/mtrr.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 26721f6ee7..cfdbcbfef1 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -814,23 +814,13 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return -1;
 }
 
-if ( direct_mmio )
-{
-if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
-return MTRR_TYPE_UNCACHABLE;
-if ( order )
-return -1;
-*ipat = 1;
-return MTRR_TYPE_WRBACK;
-}
-
 if ( !mfn_valid(mfn) )
 {
 *ipat = 1;
 return MTRR_TYPE_UNCACHABLE;
 }
 
-if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
+if ( !direct_mmio && !is_iommu_enabled(d) && !cache_flush_permitted(d) )
 {
 *ipat = 1;
 return MTRR_TYPE_WRBACK;
@@ -847,6 +837,9 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
mfn_t mfn,
 }
 }
 
+if ( direct_mmio )
+return MTRR_TYPE_UNCACHABLE;
+
 gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
 if ( gmtrr_mtype >= 0 )
 {
-- 
2.20.1

[PATCH v3 0/2] epte_get_entry_emt() modifications

[Paul Durrant]

From: Paul Durrant 

Paul Durrant (2):
  x86/hvm: set 'ipat' in EPT for special pages
  x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

 xen/arch/x86/hvm/mtrr.c | 26 +++---
 1 file changed, 15 insertions(+), 11 deletions(-)
---
Cc: Andrew Cooper 
Cc: Jan Beulich 
Cc: "Roger Pau Monné" 
Cc: Wei Liu 
-- 
2.20.1

[PATCH v3 1/2] x86/hvm: set 'ipat' in EPT for special pages

[Paul Durrant]

From: Paul Durrant 

All non-MMIO ranges (i.e those not mapping real device MMIO regions) that
map valid MFNs are normally marked MTRR_TYPE_WRBACK and 'ipat' is set. Hence
when PV drivers running in a guest populate the BAR space of the Xen Platform
PCI Device with pages such as the Shared Info page or Grant Table pages,
accesses to these pages will be cachable.

However, should IOMMU mappings be enabled be enabled for the guest then these
accesses become uncachable. This has a substantial negative effect on I/O
throughput of PV devices. Arguably PV drivers should bot be using BAR space to
host the Shared Info and Grant Table pages but it is currently commonplace for
them to do this and so this problem needs mitigation. Hence this patch makes
sure the 'ipat' bit is set for any special page regardless of where in GFN
space it is mapped.

NOTE: Clearly this mitigation only applies to Intel EPT. It is not obvious
  that there is any similar mitigation possible for AMD NPT. Downstreams
  such as Citrix XenServer have been carrying a patch similar to this for
  several releases though.

Signed-off-by: Paul Durrant 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 

v3:
 - dropping Jan's R-b
 - cope with order > 0
---
 xen/arch/x86/hvm/mtrr.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 511c3be1c8..26721f6ee7 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -836,6 +836,17 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return MTRR_TYPE_WRBACK;
 }
 
+for ( i = 0; i < (1ul << order); i++ )
+{
+if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
+{
+if ( order )
+return -1;
+*ipat = 1;
+return MTRR_TYPE_WRBACK;
+}
+}
+
 gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
 if ( gmtrr_mtype >= 0 )
 {
-- 
2.20.1

Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode

[Rafael J. Wysocki]

On Fri, Jul 31, 2020 at 4:14 PM Boris Ostrovsky
 wrote:
>
> On 7/30/20 7:06 PM, Anchal Agarwal wrote:
> > On Mon, Jul 27, 2020 at 06:08:29PM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not 
> >> click links or open attachments unless you can confirm the sender and know 
> >> the content is safe.
> >>
> >>
> >>
> >> On 7/24/20 7:01 PM, Stefano Stabellini wrote:
> >>> Yes, it does, thank you. I'd rather not introduce unknown regressions so
> >>> I would recommend to add an arch-specific check on registering
> >>> freeze/thaw/restore handlers. Maybe something like the following:
> >>>
> >>> #ifdef CONFIG_X86
> >>> .freeze = blkfront_freeze,
> >>> .thaw = blkfront_restore,
> >>> .restore = blkfront_restore
> >>> #endif
> >>>
> >>>
> >>> maybe Boris has a better suggestion on how to do it
> >>
> >> An alternative might be to still install pm notifier in
> >> drivers/xen/manage.c (I think as result of latest discussions we decided
> >> we won't need it) and return -ENOTSUPP for ARM for
> >> PM_HIBERNATION_PREPARE and friends. Would that work?
> >>
> > I think the question here is for registering driver specific 
> > freeze/thaw/restore
> > callbacks for x86 only. I have dropped the pm_notifier in the v3 still 
> > pending
> > testing. So I think just registering driver specific callbacks for x86 only 
> > is a
> > good option. What do you think?
>
>
> I suggested using the notifier under assumption that if it returns an
> error then that will prevent callbacks to be called because hibernation
> will be effectively disabled.

That's correct.

Re: [OSSTEST PATCH v2 07/41] schema: Provide indices for sg-report-flight

[George Dunlap]

> On Jul 31, 2020, at 12:37 PM, Ian Jackson  wrote:
> 
> These indexes allow very fast lookup of "relevant" flights eg when
> trying to justify failures.
> 
> In my ad-hoc test case, these indices (along with the subsequent
> changes to sg-report-flight and Executive.pm, reduce the runtime of
> sg-report-flight from 2-3ks (unacceptably long!) to as little as
> 5-7s seconds - a speedup of about 500x.
> 
> (Getting the database snapshot may take a while first, but deploying
> this code should help with that too by reducing long-running
> transactions.  Quoted perf timings are from snapshot acquisition.)
> 
> Without these new indexes there may be a performance change from the
> query changes.  I haven't benchmarked this so I am setting the schema
> updates to be Preparatory/Needed (ie, "Schema first" as
> schema/README.updates has it), to say that the index should be created
> before the new code is deployed.
> 
> Testing: I have tested this series by creating experimental indices
> "trial_..." in the actual production instance.  (Transactional DDL was
> very helpful with this.)  I have verified with \d that schema update
> instructions in this commit generate indexes which are equivalent to
> the trial indices.
> 
> Deployment: AFter these schema updates are applied, the trial indices
> are redundant duplicates and should be deleted.
> 
> CC: George Dunlap 
> Signed-off-by: Ian Jackson 

I have no idea if building an index on a LIKE is a good idea or not, but it 
certainly seems to be useful, so:

Reviewed-by: George Dunlap 

Re: [OSSTEST PATCH v2 19/41] Executive: Drop redundant AND clause

[George Dunlap]

> On Jul 31, 2020, at 12:37 PM, Ian Jackson  wrote:
> 
> In "Executive: Use index for report__find_test" we changed an EXISTS
> subquery into a JOIN.
> 
> Now, the condition r.flight=f.flight is redundant because this is the
> join column (from USING).
> 
> No functional change.
> 
> CC: George Dunlap 
> Signed-off-by: Ian Jackson 

Reviewed-by: George Dunlap 

Re: [OSSTEST PATCH v2 08/41] sg-report-flight: Ask the db for flights of interest

[George Dunlap]

> On Jul 31, 2020, at 12:37 PM, Ian Jackson  wrote:
> 
> Specifically, we narrow the initial query to flights which have at
> least some job with the built_revision_foo we are looking for.
> 
> This condition is strictly broader than that implemented inside the
> flight search loop, so there is no functional change.

Assuming this is true, that job / runvar is filtered after extracting this 
information, then...

> 
> Perf: runtime of my test case now ~300s-500s.
> 
> Example query before (from the Perl DBI trace):
> 
>  SELECT * FROM (
>SELECT flight, blessing FROM flights
>WHERE (branch='xen-unstable')
>  AND   EXISTS (SELECT 1
>FROM jobs
>   WHERE jobs.flight = flights.flight
> AND jobs.job = ?)
> 
>  AND ( (TRUE AND flight <= 151903) AND (blessing='real') )
>ORDER BY flight DESC
>LIMIT 1000
>  ) AS sub
>  ORDER BY blessing ASC, flight DESC
> 
> With these bind variables:
> 
>"test-armhf-armhf-libvirt"
> 
> After:
> 
>  SELECT * FROM (
>SELECT DISTINCT flight, blessing
> FROM flights
> JOIN runvars r1 USING (flight)
> 
>WHERE (branch='xen-unstable')
>  AND ( (TRUE AND flight <= 151903) AND (blessing='real') )
>  AND EXISTS (SELECT 1
>FROM jobs
>   WHERE jobs.flight = flights.flight
> AND jobs.job = ?)
> 
>  AND r1.name LIKE 'built\_revision\_%'
>  AND r1.name = ?
>  AND r1.val= ?
> 
>ORDER BY flight DESC
>LIMIT 1000
>  ) AS sub
>  ORDER BY blessing ASC, flight DESC

…I agree that this shoud introduce no other changes.

Reviewed-by: George Dunlap 

Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich]

On 31.07.2020 15:43, Paul Durrant wrote:
>> -Original Message-
>> From: Jan Beulich 
>> Sent: 31 July 2020 14:41
>> To: p...@xen.org
>> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' ; 
>> 'Andrew Cooper'
>> ; 'Wei Liu' ; 'Roger Pau Monné' 
>> 
>> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
>> epte_get_entry_emt()
>>
>> On 31.07.2020 15:07, Paul Durrant wrote:
 -Original Message-
 From: Jan Beulich 
 Sent: 31 July 2020 13:58
 To: Paul Durrant 
 Cc: xen-devel@lists.xenproject.org; Paul Durrant ; 
 Andrew Cooper
 ; Wei Liu ; Roger Pau Monné 
 
 Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
 epte_get_entry_emt()

 On 31.07.2020 14:39, Paul Durrant wrote:
> From: Paul Durrant 
>
> Re-factor the code to take advantage of the fact that the APIC access 
> page is
> a 'special' page.

 Hmm, that's going quite as far as I was thinking to go: In
 particular, you leave in place the set_mmio_p2m_entry() use
 in vmx_alloc_vlapic_mapping(). With that replaced, the
 re-ordering in epte_get_entry_emt() that you do shouldn't
 be necessary; you'd simple drop the checking of the
 specific MFN.
>>>
>>> Ok, it still needs to go in the p2m though so are you suggesting
>>> just calling p2m_set_entry() directly?
>>
>> Yes, if this works. The main question really is whether there are
>> any hidden assumptions elsewhere that this page gets mapped as an
>> MMIO one.
>>
> 
> Actually, it occurs to me that logdirty is going to be an issue if I
> use p2m_ram_rw. If I'm not going to use p2m_mmio_direct then do you
> have another suggestion?

p2m_ram_rw is also not good because of allowing execution. If we don't
want to create a new type, how about (ab)using p2m_grant_map_rw (in
the sense of Xen granting the domain access to this page)? Possible
problems with this are
- replace_grant_p2m_mapping()
- hvm_translate_get_page()
All other uses of p2m_grant_map_rw and p2m_is_grant() look to be
compatible with this approach.

For replace_grant_p2m_mapping() I think there's no issue because the
grant table code won't find a suitable active entry.

For hvm_translate_get_page() the question is why it checks for the
two grant types in the first place; iow I wonder whether that check
couldn't be dropped.

Independent of this, btw, you may need to call set_gpfn_from_mfn()
alongside p2m_set_entry().

Jan

Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode

[Boris Ostrovsky]

On 7/30/20 7:06 PM, Anchal Agarwal wrote:
> On Mon, Jul 27, 2020 at 06:08:29PM -0400, Boris Ostrovsky wrote:
>> CAUTION: This email originated from outside of the organization. Do not 
>> click links or open attachments unless you can confirm the sender and know 
>> the content is safe.
>>
>>
>>
>> On 7/24/20 7:01 PM, Stefano Stabellini wrote:
>>> Yes, it does, thank you. I'd rather not introduce unknown regressions so
>>> I would recommend to add an arch-specific check on registering
>>> freeze/thaw/restore handlers. Maybe something like the following:
>>>
>>> #ifdef CONFIG_X86
>>> .freeze = blkfront_freeze,
>>> .thaw = blkfront_restore,
>>> .restore = blkfront_restore
>>> #endif
>>>
>>>
>>> maybe Boris has a better suggestion on how to do it
>>
>> An alternative might be to still install pm notifier in
>> drivers/xen/manage.c (I think as result of latest discussions we decided
>> we won't need it) and return -ENOTSUPP for ARM for
>> PM_HIBERNATION_PREPARE and friends. Would that work?
>>
> I think the question here is for registering driver specific 
> freeze/thaw/restore
> callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending
> testing. So I think just registering driver specific callbacks for x86 only 
> is a
> good option. What do you think?


I suggested using the notifier under assumption that if it returns an
error then that will prevent callbacks to be called because hibernation
will be effectively disabled. But I haven't looked at PM code so I don't
know whether this is actually the case.


The advantage of doing it in the notifier is that instead of adding
ifdefs to each driver you will be able to prevent callbacks from a
single place. Plus you can use this do disable hibernation for PVH dom0
as well.



-boris

Re: [OSSTEST PATCH v2 41/41] duration_estimator: Clarify recentflights query a bit

[George Dunlap]

> On Jul 31, 2020, at 12:38 PM, Ian Jackson  wrote:
> 
> The condition on r.job is more naturally thought of as a join
> condition than a where condition.  (This is an inner join, so the
> semantics are identical.)
> 
> Also, for clarity, swap the flight and job conditions round, so that
> the ON clause is a series of r.thing = otherthing.
> 
> No functional change.
> 
> Signed-off-by: Ian Jackson 

Reviewed-by: George Dunlap 

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

> -Original Message-
> From: Paul Durrant 
> Sent: 31 July 2020 14:44
> To: 'Jan Beulich' 
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' ; 
> 'Andrew Cooper'
> ; 'Wei Liu' ; 'Roger Pau Monné' 
> 
> Subject: RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> epte_get_entry_emt()
> 
> > -Original Message-
> > From: Jan Beulich 
> > Sent: 31 July 2020 14:41
> > To: p...@xen.org
> > Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' ; 
> > 'Andrew Cooper'
> > ; 'Wei Liu' ; 'Roger Pau Monné' 
> > 
> > Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> > epte_get_entry_emt()
> >
> > On 31.07.2020 15:07, Paul Durrant wrote:
> > >> -Original Message-
> > >> From: Jan Beulich 
> > >> Sent: 31 July 2020 13:58
> > >> To: Paul Durrant 
> > >> Cc: xen-devel@lists.xenproject.org; Paul Durrant ; 
> > >> Andrew Cooper
> > >> ; Wei Liu ; Roger Pau Monné 
> > >> 
> > >> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> > >> epte_get_entry_emt()
> > >>
> > >> On 31.07.2020 14:39, Paul Durrant wrote:
> > >>> From: Paul Durrant 
> > >>>
> > >>> Re-factor the code to take advantage of the fact that the APIC access 
> > >>> page is
> > >>> a 'special' page.
> > >>
> > >> Hmm, that's going quite as far as I was thinking to go: In
> > >> particular, you leave in place the set_mmio_p2m_entry() use
> > >> in vmx_alloc_vlapic_mapping(). With that replaced, the
> > >> re-ordering in epte_get_entry_emt() that you do shouldn't
> > >> be necessary; you'd simple drop the checking of the
> > >> specific MFN.
> > >
> > > Ok, it still needs to go in the p2m though so are you suggesting
> > > just calling p2m_set_entry() directly?
> >
> > Yes, if this works. The main question really is whether there are
> > any hidden assumptions elsewhere that this page gets mapped as an
> > MMIO one.
> >
> 
> Actually, it occurs to me that logdirty is going to be an issue if I use 
> p2m_ram_rw. If I'm not going
> to use p2m_mmio_direct then do you have another suggestion?
> 

Looking further I'm uneasy to move away from setting that APIC access page to 
anything other than mmio_direct so I'd rather leave the VMX code alone and 
re-order things here.

  Paul

Re: [PATCH] x86/vmx: reorder code in vmx_deliver_posted_intr

[Roger Pau Monné]

On Fri, Jul 31, 2020 at 03:05:52PM +0200, Jan Beulich wrote:
> On 30.07.2020 16:03, Roger Pau Monne wrote:
> > Remove the unneeded else branch, which allows to reduce the
> > indentation of a larger block of code, while making the flow of the
> > function more obvious.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné 
> 
> Reviewed-by: Jan Beulich 
> 
> One minor request (could likely be taken care of while
> committing):
> 
> > @@ -2014,41 +2016,36 @@ static void vmx_deliver_posted_intr(struct vcpu *v, 
> > u8 vector)
> >   * VMEntry as it used to be.
> >   */
> >  pi_set_on(&v->arch.hvm.vmx.pi_desc);
> > +vcpu_kick(v);
> > +return;
> >  }
> > -else
> > -{
> > -struct pi_desc old, new, prev;
> >  
> > -prev.control = v->arch.hvm.vmx.pi_desc.control;
> > +prev.control = v->arch.hvm.vmx.pi_desc.control;
> >  
> > -do {
> > -/*
> > - * Currently, we don't support urgent interrupt, all
> > - * interrupts are recognized as non-urgent interrupt,
> > - * Besides that, if 'ON' is already set, no need to
> > - * sent posted-interrupts notification event as well,
> > - * according to hardware behavior.
> > - */
> > -if ( pi_test_sn(&prev) || pi_test_on(&prev) )
> > -{
> > -vcpu_kick(v);
> > -return;
> > -}
> > -
> > -old.control = v->arch.hvm.vmx.pi_desc.control &
> > -  ~((1 << POSTED_INTR_ON) | (1 << POSTED_INTR_SN));
> > -new.control = v->arch.hvm.vmx.pi_desc.control |
> > -  (1 << POSTED_INTR_ON);
> > +do {
> > +/*
> > + * Currently, we don't support urgent interrupt, all
> > + * interrupts are recognized as non-urgent interrupt,
> > + * Besides that, if 'ON' is already set, no need to
> > + * sent posted-interrupts notification event as well,
> > + * according to hardware behavior.
> > + */
> 
> Would be nice to s/sent/send/ here as you move it (maybe also
> remove the plural from "posted-interrupts") and - if possible -
> re-flow for the now increased space on the right side.

Oh, sure, I should have realized myself. Feel free to adjust at commit
if you don't mind. I would also adjust 'non-urgent interrupts'.

Thanks, Roger.

Re: kernel-doc and xen.git

[Bertrand Marquis]

> On 31 Jul 2020, at 15:48, Andrew Cooper  wrote:
> 
> On 30/07/2020 02:27, Stefano Stabellini wrote:
>> Hi all,
>> 
>> I would like to ask for your feedback on the adoption of the kernel-doc
>> format for in-code comments.
>> 
>> In the FuSa SIG we have started looking into FuSa documents for Xen. One
>> of the things we are investigating are ways to link these documents to
>> in-code comments in xen.git and vice versa.
>> 
>> In this context, Andrew Cooper suggested to have a look at "kernel-doc"
>> [1] during one of the virtual beer sessions at the last Xen Summit.
>> 
>> I did give a look at kernel-doc and it is very promising. kernel-doc is
>> a script that can generate nice rst text documents from in-code
>> comments. (The generated rst files can then be used as input for sphinx
>> to generate html docs.) The comment syntax [2] is simple and similar to
>> Doxygen:
>> 
>>/**
>> * function_name() - Brief description of function.
>> * @arg1: Describe the first argument.
>> * @arg2: Describe the second argument.
>> *One can provide multiple line descriptions
>> *for arguments.
>> */
>> 
>> kernel-doc is actually better than Doxygen because it is a much simpler
>> tool, one we could customize to our needs and with predictable output.
>> Specifically, we could add the tagging, numbering, and referencing
>> required by FuSa requirement documents.
>> 
>> I would like your feedback on whether it would be good to start
>> converting xen.git in-code comments to the kernel-doc format so that
>> proper documents can be generated out of them. One day we could import
>> kernel-doc into xen.git/scripts and use it to generate a set of html
>> documents via sphinx.
>> 
>> At a minimum we'll need to start the in-code comment blocks with two
>> stars:
>> 
>>/**
>> 
>> There could be also other small changes required to make sure the output
>> is appropriate.
>> 
>> 
>> Feedback is welcome!
> 
> I think it goes without saying that I'm +1 to this in principle.
> 
> We definitely have some /** comments already, but I have no idea if they
> are valid kernel-doc or not.  It seems that the kernel-doc has some
> ability to report warnings, so it would be interesting to see what that
> spits out.

From my first crash test, not much is “kernel-doc” friendly but the content
is there, it is only a matter of doing some formatting.

> 
> I also think getting rid of our home-grown syntax for the public headers
> will be major improvement.  We actually have a reasonable amount of
> ancillary documentation
> 
> As with everything else in the Sphinx docs, I'd request that we don't
> just blindly throw this in and call it done.  We need to curate
> additions properly to avoid the docs turning into a mess.  I'm happy to
> help out in my copious free time.

Thanks :-)

Bertrand

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

> -Original Message-
> From: Jan Beulich 
> Sent: 31 July 2020 14:41
> To: p...@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' ; 
> 'Andrew Cooper'
> ; 'Wei Liu' ; 'Roger Pau Monné' 
> 
> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> epte_get_entry_emt()
> 
> On 31.07.2020 15:07, Paul Durrant wrote:
> >> -Original Message-
> >> From: Jan Beulich 
> >> Sent: 31 July 2020 13:58
> >> To: Paul Durrant 
> >> Cc: xen-devel@lists.xenproject.org; Paul Durrant ; 
> >> Andrew Cooper
> >> ; Wei Liu ; Roger Pau Monné 
> >> 
> >> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> >> epte_get_entry_emt()
> >>
> >> On 31.07.2020 14:39, Paul Durrant wrote:
> >>> From: Paul Durrant 
> >>>
> >>> Re-factor the code to take advantage of the fact that the APIC access 
> >>> page is
> >>> a 'special' page.
> >>
> >> Hmm, that's going quite as far as I was thinking to go: In
> >> particular, you leave in place the set_mmio_p2m_entry() use
> >> in vmx_alloc_vlapic_mapping(). With that replaced, the
> >> re-ordering in epte_get_entry_emt() that you do shouldn't
> >> be necessary; you'd simple drop the checking of the
> >> specific MFN.
> >
> > Ok, it still needs to go in the p2m though so are you suggesting
> > just calling p2m_set_entry() directly?
> 
> Yes, if this works. The main question really is whether there are
> any hidden assumptions elsewhere that this page gets mapped as an
> MMIO one.
>

Actually, it occurs to me that logdirty is going to be an issue if I use 
p2m_ram_rw. If I'm not going to use p2m_mmio_direct then do you have another 
suggestion?

  Paul

Re: kernel-doc and xen.git

[Andrew Cooper]

On 30/07/2020 02:27, Stefano Stabellini wrote:
> Hi all,
>
> I would like to ask for your feedback on the adoption of the kernel-doc
> format for in-code comments.
>
> In the FuSa SIG we have started looking into FuSa documents for Xen. One
> of the things we are investigating are ways to link these documents to
> in-code comments in xen.git and vice versa.
>
> In this context, Andrew Cooper suggested to have a look at "kernel-doc"
> [1] during one of the virtual beer sessions at the last Xen Summit.
>
> I did give a look at kernel-doc and it is very promising. kernel-doc is
> a script that can generate nice rst text documents from in-code
> comments. (The generated rst files can then be used as input for sphinx
> to generate html docs.) The comment syntax [2] is simple and similar to
> Doxygen:
>
> /**
>  * function_name() - Brief description of function.
>  * @arg1: Describe the first argument.
>  * @arg2: Describe the second argument.
>  *One can provide multiple line descriptions
>  *for arguments.
>  */
>
> kernel-doc is actually better than Doxygen because it is a much simpler
> tool, one we could customize to our needs and with predictable output.
> Specifically, we could add the tagging, numbering, and referencing
> required by FuSa requirement documents.
>
> I would like your feedback on whether it would be good to start
> converting xen.git in-code comments to the kernel-doc format so that
> proper documents can be generated out of them. One day we could import
> kernel-doc into xen.git/scripts and use it to generate a set of html
> documents via sphinx.
>
> At a minimum we'll need to start the in-code comment blocks with two
> stars:
>
> /**
>
> There could be also other small changes required to make sure the output
> is appropriate.
>
>
> Feedback is welcome!

I think it goes without saying that I'm +1 to this in principle.

We definitely have some /** comments already, but I have no idea if they
are valid kernel-doc or not.  It seems that the kernel-doc has some
ability to report warnings, so it would be interesting to see what that
spits out.

I also think getting rid of our home-grown syntax for the public headers
will be major improvement.  We actually have a reasonable amount of
ancillary documentation

As with everything else in the Sphinx docs, I'd request that we don't
just blindly throw this in and call it done.  We need to curate
additions properly to avoid the docs turning into a mess.  I'm happy to
help out in my copious free time.

~Andrew

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Jan Beulich]

On 31.07.2020 15:46, Andrew Cooper wrote:
> On 31/07/2020 14:30, Jan Beulich wrote:
>> On 31.07.2020 15:02, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
>>> variable"):
 On 29.06.2020 14:05, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
> variable"):
>> On 26.06.2020 19:00, Andrew Cooper wrote:
>> ... this may or may not take effect on the file system the sources
>> are stored on.
> In what circumstances might this not take effect ?
 When the file system is incapable of recording execute permissions?
 It has been a common workaround for this in various projects that
 I've worked with to use $(SHELL) to account for that, so the actual
 permissions from the fs don't matter. (There may be mount options
 to make everything executable on such file systems, but people may
 be hesitant to use them.)
>>> I don't think we support building from sources which have been
>>> unpacked onto such filesystems.  Other projects which might actually
>>> need to build on Windows or something do do this $(SHELL) thing or an
>>> equivalent, but I don't think that's us.
>> It's not unexpected that you think of Windows here, but my thoughts
>> were more towards building from sources on a CD or DVD, where iirc
>> execute permissions also don't exist. The latest when we have
>> out-of-tree builds fully working, this ought to be something that
>> people should be able to do, imo. (Even without out-of-tree builds,
>> my "next best" alternative of using a tree of symlinks to build in
>> would similarly have an issue with the links pointing at a mounted
>> CD/DVD, if the $(SHELL) wasn't present.)
> 
> See v2.  I put $(SHELL) in, because it isn't a worthwhile use of our
> time to continue arguing over this point.

I had seen you did; I was merely replying back to Ian's comments.

Jan

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Andrew Cooper]

On 31/07/2020 14:30, Jan Beulich wrote:
> On 31.07.2020 15:02, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
>> variable"):
>>> On 29.06.2020 14:05, Ian Jackson wrote:
 Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
 variable"):
> On 26.06.2020 19:00, Andrew Cooper wrote:
> ... this may or may not take effect on the file system the sources
> are stored on.
 In what circumstances might this not take effect ?
>>> When the file system is incapable of recording execute permissions?
>>> It has been a common workaround for this in various projects that
>>> I've worked with to use $(SHELL) to account for that, so the actual
>>> permissions from the fs don't matter. (There may be mount options
>>> to make everything executable on such file systems, but people may
>>> be hesitant to use them.)
>> I don't think we support building from sources which have been
>> unpacked onto such filesystems.  Other projects which might actually
>> need to build on Windows or something do do this $(SHELL) thing or an
>> equivalent, but I don't think that's us.
> It's not unexpected that you think of Windows here, but my thoughts
> were more towards building from sources on a CD or DVD, where iirc
> execute permissions also don't exist. The latest when we have
> out-of-tree builds fully working, this ought to be something that
> people should be able to do, imo. (Even without out-of-tree builds,
> my "next best" alternative of using a tree of symlinks to build in
> would similarly have an issue with the links pointing at a mounted
> CD/DVD, if the $(SHELL) wasn't present.)

See v2.  I put $(SHELL) in, because it isn't a worthwhile use of our
time to continue arguing over this point.

~Andrew

Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich]

On 31.07.2020 15:07, Paul Durrant wrote:
>> -Original Message-
>> From: Jan Beulich 
>> Sent: 31 July 2020 13:58
>> To: Paul Durrant 
>> Cc: xen-devel@lists.xenproject.org; Paul Durrant ; 
>> Andrew Cooper
>> ; Wei Liu ; Roger Pau Monné 
>> 
>> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
>> epte_get_entry_emt()
>>
>> On 31.07.2020 14:39, Paul Durrant wrote:
>>> From: Paul Durrant 
>>>
>>> Re-factor the code to take advantage of the fact that the APIC access page 
>>> is
>>> a 'special' page.
>>
>> Hmm, that's going quite as far as I was thinking to go: In
>> particular, you leave in place the set_mmio_p2m_entry() use
>> in vmx_alloc_vlapic_mapping(). With that replaced, the
>> re-ordering in epte_get_entry_emt() that you do shouldn't
>> be necessary; you'd simple drop the checking of the
>> specific MFN.
> 
> Ok, it still needs to go in the p2m though so are you suggesting
> just calling p2m_set_entry() directly?

Yes, if this works. The main question really is whether there are
any hidden assumptions elsewhere that this page gets mapped as an
MMIO one.

>>> --- a/xen/arch/x86/hvm/mtrr.c
>>> +++ b/xen/arch/x86/hvm/mtrr.c
>>> @@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned 
>>> long gfn, mfn_t mfn,
>>>  return -1;
>>>  }
>>>
>>> -if ( direct_mmio )
>>> -{
>>> -if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> 
>>> order )
>>> -return MTRR_TYPE_UNCACHABLE;
>>> -if ( order )
>>> -return -1;
>>
>> ... this part of the logic wants retaining, I think, i.e.
>> reporting back to the guest that the mapping needs splitting.
>> I'm afraid I have to withdraw my R-b on patch 1 for this
>> reason, as the check needs to be added there already.
> 
> To be clear... You mean I need the:
> 
> if ( order )
>   return -1;
> 
> there?

Not just - first of all you need to check whether the requested
range overlaps a special page.

Jan

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Jan Beulich]

On 31.07.2020 15:02, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
> variable"):
>> On 29.06.2020 14:05, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
>>> variable"):
 On 26.06.2020 19:00, Andrew Cooper wrote:
 ... this may or may not take effect on the file system the sources
 are stored on.
>>>
>>> In what circumstances might this not take effect ?
>>
>> When the file system is incapable of recording execute permissions?
>> It has been a common workaround for this in various projects that
>> I've worked with to use $(SHELL) to account for that, so the actual
>> permissions from the fs don't matter. (There may be mount options
>> to make everything executable on such file systems, but people may
>> be hesitant to use them.)
> 
> I don't think we support building from sources which have been
> unpacked onto such filesystems.  Other projects which might actually
> need to build on Windows or something do do this $(SHELL) thing or an
> equivalent, but I don't think that's us.

It's not unexpected that you think of Windows here, but my thoughts
were more towards building from sources on a CD or DVD, where iirc
execute permissions also don't exist. The latest when we have
out-of-tree builds fully working, this ought to be something that
people should be able to do, imo. (Even without out-of-tree builds,
my "next best" alternative of using a tree of symlinks to build in
would similarly have an issue with the links pointing at a mounted
CD/DVD, if the $(SHELL) wasn't present.)

Jan

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis]

Sorry missed some points in my previous answer.

> On 30 Jul 2020, at 22:50, Julien Grall  wrote:
> 
> Hi Bertrand,
> 
> To avoid extra work on your side, I would recommend to wait a bit before 
> sending a new version. It would be good to at least settle the conversation 
> in v2 regarding the approach taken.
> 
> On 30/07/2020 11:24, Bertrand Marquis wrote:
>> At the moment on Arm, a Linux guest running with KTPI enabled will
>> cause the following error when a context switch happens in user mode:
>> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0
>> The error is caused by the virtual address for the runstate area
>> registered by the guest only being accessible when the guest is running
>> in kernel space when KPTI is enabled.
>> To solve this issue, this patch is doing the translation from virtual
>> address to physical address during the hypercall and mapping the
>> required pages using vmap. This is removing the conversion from virtual
>> to physical address during the context switch which is solving the
>> problem with KPTI.
> 
> To echo what Jan said on the previous version, this is a change in a stable 
> ABI and therefore may break existing guest. FAOD, I agree in principle with 
> the idea. However, we want to explain why breaking the ABI is the *only* 
> viable solution.
> 
> From my understanding, it is not possible to fix without an ABI breakage 
> because the hypervisor doesn't know when the guest will switch back from 
> userspace to kernel space. The risk is the information provided by the 
> runstate wouldn't contain accurate information and could affect how the guest 
> handle stolen time.
> 
> Additionally there are a few issues with the current interface:
>   1) It is assuming the virtual address cannot be re-used by the userspace. 
> Thanksfully Linux have a split address space. But this may change with KPTI 
> in place.
>   2) When update the page-tables, the guest has to go through an invalid 
> mapping. So the translation may fail at any point.
> 
> IOW, the existing interface can lead to random memory corruption and 
> inacurracy of the stolen time.
> 
>> This is done only on arm architecture, the behaviour on x86 is not
>> modified by this patch and the address conversion is done as before
>> during each context switch.
>> This is introducing several limitations in comparison to the previous
>> behaviour (on arm only):
>> - if the guest is remapping the area at a different physical address Xen
>> will continue to update the area at the previous physical address. As
>> the area is in kernel space and usually defined as a global variable this
>> is something which is believed not to happen. If this is required by a
>> guest, it will have to call the hypercall with the new area (even if it
>> is at the same virtual address).
>> - the area needs to be mapped during the hypercall. For the same reasons
>> as for the previous case, even if the area is registered for a different
>> vcpu. It is believed that registering an area using a virtual address
>> unmapped is not something done.
> 
> This is not clear whether the virtual address refer to the current vCPU or 
> the vCPU you register the runstate for. From the past discussion, I think you 
> refer to the former. It would be good to clarify.
> 
> Additionally, all the new restrictions should be documented in the public 
> interface. So an OS developper can find the differences between the 
> architectures.
> 
> To answer Jan's concern, we certainly don't know all the guest OSes existing, 
> however we also need to balance the benefit for a large majority of the users.
> 
> From previous discussion, the current approach was deemed to be acceptable on 
> Arm and, AFAICT, also x86 (see [1]).
> 
> TBH, I would rather see the approach to be common. For that, we would an 
> agreement from Andrew and Jan in the approach here. Meanwhile, I think this 
> is the best approach to address the concern from Arm users.
> 
>> inline functions in headers could not be used as the architecture
>> domain.h is included before the global domain.h making it impossible
>> to use the struct vcpu inside the architecture header.
>> This should not have any performance impact as the hypercall is only
>> called once per vcpu usually.
>> Signed-off-by: Bertrand Marquis 
>> ---
>>   Changes in v2
>> - use vmap to map the pages during the hypercall.
>> - reintroduce initial copy during hypercall.
>>   Changes in v3
>> - Fix Coding style
>> - Fix vaddr printing on arm32
>> - use write_atomic to modify state_entry_time update bit (only
>> in guest structure as the bit is not used inside Xen copy)
>> ---
>>  xen/arch/arm/domain.c| 161 ++-
>>  xen/arch/x86/domain.c|  29 ++-
>>  xen/arch/x86/x86_64/domain.c |   4 +-
>>  xen/common/domain.c  |  19 ++---
>>  xen/include/asm-arm/domain.h |   9 ++
>>  xen/include/asm-x86/domain.h |  16 
>>  xen/include/xen/domain.h

RE: [PATCH v2 08/10] remove remaining uses of iommu_legacy_map/unmap

[Durrant, Paul]

> -Original Message-
> From: Paul Durrant 
> Sent: 30 July 2020 15:29
> To: xen-devel@lists.xenproject.org
> Cc: Durrant, Paul ; Jan Beulich ; 
> Andrew Cooper
> ; Wei Liu ; Roger Pau Monné 
> ; George
> Dunlap ; Ian Jackson ; 
> Julien Grall
> ; Stefano Stabellini ; Jun Nakajima 
> ;
> Kevin Tian 
> Subject: [EXTERNAL] [PATCH v2 08/10] remove remaining uses of 
> iommu_legacy_map/unmap
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> From: Paul Durrant 
> 
> The 'legacy' functions do implicit flushing so amend the callers to do the
> appropriate flushing.
> 
> Unfortunately, because of the structure of the P2M code, we cannot remove
> the per-CPU 'iommu_dont_flush_iotlb' global and the optimization it
> facilitates. It is now checked directly iommu_iotlb_flush(). Also, it is
> now declared as bool (rather than bool_t) and setting/clearing it are no
> longer pointlessly gated on is_iommu_enabled() returning true. (Arguably
> it is also pointless to gate the call to iommu_iotlb_flush() on that
> condition - since it is a no-op in that case - but the if clause allows
> the scope of a stack variable to be restricted).
> 
> NOTE: The code in memory_add() now fails if the number of pages passed to
>   a single call overflows an unsigned int. I don't believe this will
>   ever happen in practice.
> 
> Signed-off-by: Paul Durrant 

I realise now that I completely forgot to address Jan's comments on grant table 
locking and flush batching, so there will be a v3 of at least this patch.

  Paul

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis]

> On 31 Jul 2020, at 03:18, Stefano Stabellini  wrote:
> 
> On Thu, 30 Jul 2020, Julien Grall wrote:
>> Hi Bertrand,
>> 
>> To avoid extra work on your side, I would recommend to wait a bit before
>> sending a new version. It would be good to at least settle the conversation 
>> in
>> v2 regarding the approach taken.
>> 
>> On 30/07/2020 11:24, Bertrand Marquis wrote:
>>> At the moment on Arm, a Linux guest running with KTPI enabled will
>>> cause the following error when a context switch happens in user mode:
>>> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0
>>> 
>>> The error is caused by the virtual address for the runstate area
>>> registered by the guest only being accessible when the guest is running
>>> in kernel space when KPTI is enabled.
>>> 
>>> To solve this issue, this patch is doing the translation from virtual
>>> address to physical address during the hypercall and mapping the
>>> required pages using vmap. This is removing the conversion from virtual
>>> to physical address during the context switch which is solving the
>>> problem with KPTI.
>> 
>> To echo what Jan said on the previous version, this is a change in a stable
>> ABI and therefore may break existing guest. FAOD, I agree in principle with
>> the idea. However, we want to explain why breaking the ABI is the *only*
>> viable solution.
>> 
>> From my understanding, it is not possible to fix without an ABI breakage
>> because the hypervisor doesn't know when the guest will switch back from
>> userspace to kernel space. The risk is the information provided by the
>> runstate wouldn't contain accurate information and could affect how the guest
>> handle stolen time.
>> 
>> Additionally there are a few issues with the current interface:
>>   1) It is assuming the virtual address cannot be re-used by the userspace.
>> Thanksfully Linux have a split address space. But this may change with KPTI 
>> in
>> place.
>>   2) When update the page-tables, the guest has to go through an invalid
>> mapping. So the translation may fail at any point.
>> 
>> IOW, the existing interface can lead to random memory corruption and
>> inacurracy of the stolen time.
>> 
>>> 
>>> This is done only on arm architecture, the behaviour on x86 is not
>>> modified by this patch and the address conversion is done as before
>>> during each context switch.
>>> 
>>> This is introducing several limitations in comparison to the previous
>>> behaviour (on arm only):
>>> - if the guest is remapping the area at a different physical address Xen
>>> will continue to update the area at the previous physical address. As
>>> the area is in kernel space and usually defined as a global variable this
>>> is something which is believed not to happen. If this is required by a
>>> guest, it will have to call the hypercall with the new area (even if it
>>> is at the same virtual address).
>>> - the area needs to be mapped during the hypercall. For the same reasons
>>> as for the previous case, even if the area is registered for a different
>>> vcpu. It is believed that registering an area using a virtual address
>>> unmapped is not something done.
>> 
>> This is not clear whether the virtual address refer to the current vCPU or 
>> the
>> vCPU you register the runstate for. From the past discussion, I think you
>> refer to the former. It would be good to clarify.
>> 
>> Additionally, all the new restrictions should be documented in the public
>> interface. So an OS developper can find the differences between the
>> architectures.
> 
> Just to paraphrase what Julien wrote, it would be good to improve the
> commit message with the points suggested and also write a note in the
> header file about the changes to the interface.

Ok i wil do that.

> 
> 
>> To answer Jan's concern, we certainly don't know all the guest OSes existing,
>> however we also need to balance the benefit for a large majority of the 
>> users.
>> 
>> From previous discussion, the current approach was deemed to be acceptable on
>> Arm and, AFAICT, also x86 (see [1]).
>> 
>> TBH, I would rather see the approach to be common. For that, we would an
>> agreement from Andrew and Jan in the approach here. Meanwhile, I think this 
>> is
>> the best approach to address the concern from Arm users.
> 
> +1
> 
> 
>>> inline functions in headers could not be used as the architecture
>>> domain.h is included before the global domain.h making it impossible
>>> to use the struct vcpu inside the architecture header.
>>> This should not have any performance impact as the hypercall is only
>>> called once per vcpu usually.
>>> 
>>> Signed-off-by: Bertrand Marquis 
>>> 
>>> ---
>>>   Changes in v2
>>> - use vmap to map the pages during the hypercall.
>>> - reintroduce initial copy during hypercall.
>>> 
>>>   Changes in v3
>>> - Fix Coding style
>>> - Fix vaddr printing on arm32
>>> - use write_atomic to modify state_entry_time update bit (only
>>> in guest structure as the bit is n

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis]

> On 30 Jul 2020, at 22:50, Julien Grall  wrote:
> 
> Hi Bertrand,
> 
> To avoid extra work on your side, I would recommend to wait a bit before 
> sending a new version. It would be good to at least settle the conversation 
> in v2 regarding the approach taken.

Thanks for the advice.
I thought all discussions where done the first time we started the discussion 
on the last thread but there are apparently more point to go through.

> 
> On 30/07/2020 11:24, Bertrand Marquis wrote:
>> At the moment on Arm, a Linux guest running with KTPI enabled will
>> cause the following error when a context switch happens in user mode:
>> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0
>> The error is caused by the virtual address for the runstate area
>> registered by the guest only being accessible when the guest is running
>> in kernel space when KPTI is enabled.
>> To solve this issue, this patch is doing the translation from virtual
>> address to physical address during the hypercall and mapping the
>> required pages using vmap. This is removing the conversion from virtual
>> to physical address during the context switch which is solving the
>> problem with KPTI.
> 
> To echo what Jan said on the previous version, this is a change in a stable 
> ABI and therefore may break existing guest. FAOD, I agree in principle with 
> the idea. However, we want to explain why breaking the ABI is the *only* 
> viable solution.
> 
> From my understanding, it is not possible to fix without an ABI breakage 
> because the hypervisor doesn't know when the guest will switch back from 
> userspace to kernel space. The risk is the information provided by the 
> runstate wouldn't contain accurate information and could affect how the guest 
> handle stolen time.
> 
> Additionally there are a few issues with the current interface:
>   1) It is assuming the virtual address cannot be re-used by the userspace. 
> Thanksfully Linux have a split address space. But this may change with KPTI 
> in place.
>   2) When update the page-tables, the guest has to go through an invalid 
> mapping. So the translation may fail at any point.
> 
> IOW, the existing interface can lead to random memory corruption and 
> inacurracy of the stolen time.

I agree but i am not sure what you want me to do here.
Should i add more details in the commit message ?

> 
>> This is done only on arm architecture, the behaviour on x86 is not
>> modified by this patch and the address conversion is done as before
>> during each context switch.
>> This is introducing several limitations in comparison to the previous
>> behaviour (on arm only):
>> - if the guest is remapping the area at a different physical address Xen
>> will continue to update the area at the previous physical address. As
>> the area is in kernel space and usually defined as a global variable this
>> is something which is believed not to happen. If this is required by a
>> guest, it will have to call the hypercall with the new area (even if it
>> is at the same virtual address).
>> - the area needs to be mapped during the hypercall. For the same reasons
>> as for the previous case, even if the area is registered for a different
>> vcpu. It is believed that registering an area using a virtual address
>> unmapped is not something done.
> 
> This is not clear whether the virtual address refer to the current vCPU or 
> the vCPU you register the runstate for. From the past discussion, I think you 
> refer to the former. It would be good to clarify.

Ok i will try to clarify.

> 
> Additionally, all the new restrictions should be documented in the public 
> interface. So an OS developper can find the differences between the 
> architectures.
> 
> To answer Jan's concern, we certainly don't know all the guest OSes existing, 
> however we also need to balance the benefit for a large majority of the users.
> 
> From previous discussion, the current approach was deemed to be acceptable on 
> Arm and, AFAICT, also x86 (see [1]).
> 
> TBH, I would rather see the approach to be common. For that, we would an 
> agreement from Andrew and Jan in the approach here. Meanwhile, I think this 
> is the best approach to address the concern from Arm users.

>From this I get that you want me to document the specific behaviour on Arm on 
>the public header describing the hypercall, right ?

Bertrand

> 
>> inline functions in headers could not be used as the architecture
>> domain.h is included before the global domain.h making it impossible
>> to use the struct vcpu inside the architecture header.
>> This should not have any performance impact as the hypercall is only
>> called once per vcpu usually.
>> Signed-off-by: Bertrand Marquis 
>> ---
>>   Changes in v2
>> - use vmap to map the pages during the hypercall.
>> - reintroduce initial copy during hypercall.
>>   Changes in v3
>> - Fix Coding style
>> - Fix vaddr printing on arm32
>> - use write_atomic to modify state_entry_time updat

RE: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

> -Original Message-
> From: Jan Beulich 
> Sent: 31 July 2020 13:58
> To: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org; Paul Durrant ; 
> Andrew Cooper
> ; Wei Liu ; Roger Pau Monné 
> 
> Subject: Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in 
> epte_get_entry_emt()
> 
> On 31.07.2020 14:39, Paul Durrant wrote:
> > From: Paul Durrant 
> >
> > Re-factor the code to take advantage of the fact that the APIC access page 
> > is
> > a 'special' page.
> 
> Hmm, that's going quite as far as I was thinking to go: In
> particular, you leave in place the set_mmio_p2m_entry() use
> in vmx_alloc_vlapic_mapping(). With that replaced, the
> re-ordering in epte_get_entry_emt() that you do shouldn't
> be necessary; you'd simple drop the checking of the
> specific MFN.

Ok, it still needs to go in the p2m though so are you suggesting just calling 
p2m_set_entry() directly?

> However, ...
> 
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned 
> > long gfn, mfn_t mfn,
> >  return -1;
> >  }
> >
> > -if ( direct_mmio )
> > -{
> > -if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> 
> > order )
> > -return MTRR_TYPE_UNCACHABLE;
> > -if ( order )
> > -return -1;
> 
> ... this part of the logic wants retaining, I think, i.e.
> reporting back to the guest that the mapping needs splitting.
> I'm afraid I have to withdraw my R-b on patch 1 for this
> reason, as the check needs to be added there already.

To be clear... You mean I need the:

if ( order )
  return -1;

there?

  Paul

> 
> Jan

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Bertrand Marquis]

> On 31 Jul 2020, at 14:19, Jan Beulich  wrote:
> 
> On 30.07.2020 22:50, Julien Grall wrote:
>> On 30/07/2020 11:24, Bertrand Marquis wrote:
>>> At the moment on Arm, a Linux guest running with KTPI enabled will
>>> cause the following error when a context switch happens in user mode:
>>> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0
>>> 
>>> The error is caused by the virtual address for the runstate area
>>> registered by the guest only being accessible when the guest is running
>>> in kernel space when KPTI is enabled.
>>> 
>>> To solve this issue, this patch is doing the translation from virtual
>>> address to physical address during the hypercall and mapping the
>>> required pages using vmap. This is removing the conversion from virtual
>>> to physical address during the context switch which is solving the
>>> problem with KPTI.
>> 
>> To echo what Jan said on the previous version, this is a change in a 
>> stable ABI and therefore may break existing guest. FAOD, I agree in 
>> principle with the idea. However, we want to explain why breaking the 
>> ABI is the *only* viable solution.
>> 
>> From my understanding, it is not possible to fix without an ABI 
>> breakage because the hypervisor doesn't know when the guest will switch 
>> back from userspace to kernel space.
> 
> And there's also no way to know on Arm, by e.g. enabling a suitable
> intercept?

An intercept would mean that Xen gets a notice whenever a guest is switching
from kernel mode to user mode.
There is nothing in this process which could be intercepted by Xen, appart from
maybe trapping all access to MMU registers which would be very complex and
slow.

> 
>> The risk is the information 
>> provided by the runstate wouldn't contain accurate information and could 
>> affect how the guest handle stolen time.
>> 
>> Additionally there are a few issues with the current interface:
>>1) It is assuming the virtual address cannot be re-used by the 
>> userspace. Thanksfully Linux have a split address space. But this may 
>> change with KPTI in place.
>>2) When update the page-tables, the guest has to go through an 
>> invalid mapping. So the translation may fail at any point.
>> 
>> IOW, the existing interface can lead to random memory corruption and 
>> inacurracy of the stolen time.
>> 
>>> 
>>> This is done only on arm architecture, the behaviour on x86 is not
>>> modified by this patch and the address conversion is done as before
>>> during each context switch.
>>> 
>>> This is introducing several limitations in comparison to the previous
>>> behaviour (on arm only):
>>> - if the guest is remapping the area at a different physical address Xen
>>> will continue to update the area at the previous physical address. As
>>> the area is in kernel space and usually defined as a global variable this
>>> is something which is believed not to happen. If this is required by a
>>> guest, it will have to call the hypercall with the new area (even if it
>>> is at the same virtual address).
>>> - the area needs to be mapped during the hypercall. For the same reasons
>>> as for the previous case, even if the area is registered for a different
>>> vcpu. It is believed that registering an area using a virtual address
>>> unmapped is not something done.
>> 
>> This is not clear whether the virtual address refer to the current vCPU 
>> or the vCPU you register the runstate for. From the past discussion, I 
>> think you refer to the former. It would be good to clarify.
>> 
>> Additionally, all the new restrictions should be documented in the 
>> public interface. So an OS developper can find the differences between 
>> the architectures.
>> 
>> To answer Jan's concern, we certainly don't know all the guest OSes 
>> existing, however we also need to balance the benefit for a large 
>> majority of the users.
>> 
>> From previous discussion, the current approach was deemed to be 
>> acceptable on Arm and, AFAICT, also x86 (see [1]).
>> 
>> TBH, I would rather see the approach to be common. For that, we would an 
>> agreement from Andrew and Jan in the approach here. Meanwhile, I think 
>> this is the best approach to address the concern from Arm users.
> 
> Just FTR: If x86 was to also change, VCPUOP_register_vcpu_time_memory_area
> would need taking care of as well, as it's using the same underlying model
> (including recovery logic when, while the guest is in user mode, the
> update has failed).
> 
>>> @@ -275,36 +276,156 @@ static void ctxt_switch_to(struct vcpu *n)
>>>  virt_timer_restore(n);
>>>  }
>>> 
>>> -/* Update per-VCPU guest runstate shared memory area (if registered). */
>>> -static void update_runstate_area(struct vcpu *v)
>>> +static void cleanup_runstate_vcpu_locked(struct vcpu *v)
>>>  {
>>> -void __user *guest_handle = NULL;
>>> +if ( v->arch.runstate_guest )
>>> +{
>>> +vunmap((void *)((unsigned long)v->arch.runstate_guest & 
>>> PAGE_MASK));
>>> +
>>> +put_page(v->arch.runs

Re: [PATCH] x86/vmx: reorder code in vmx_deliver_posted_intr

[Jan Beulich]

On 30.07.2020 16:03, Roger Pau Monne wrote:
> Remove the unneeded else branch, which allows to reduce the
> indentation of a larger block of code, while making the flow of the
> function more obvious.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné 

Reviewed-by: Jan Beulich 

One minor request (could likely be taken care of while
committing):

> @@ -2014,41 +2016,36 @@ static void vmx_deliver_posted_intr(struct vcpu *v, 
> u8 vector)
>   * VMEntry as it used to be.
>   */
>  pi_set_on(&v->arch.hvm.vmx.pi_desc);
> +vcpu_kick(v);
> +return;
>  }
> -else
> -{
> -struct pi_desc old, new, prev;
>  
> -prev.control = v->arch.hvm.vmx.pi_desc.control;
> +prev.control = v->arch.hvm.vmx.pi_desc.control;
>  
> -do {
> -/*
> - * Currently, we don't support urgent interrupt, all
> - * interrupts are recognized as non-urgent interrupt,
> - * Besides that, if 'ON' is already set, no need to
> - * sent posted-interrupts notification event as well,
> - * according to hardware behavior.
> - */
> -if ( pi_test_sn(&prev) || pi_test_on(&prev) )
> -{
> -vcpu_kick(v);
> -return;
> -}
> -
> -old.control = v->arch.hvm.vmx.pi_desc.control &
> -  ~((1 << POSTED_INTR_ON) | (1 << POSTED_INTR_SN));
> -new.control = v->arch.hvm.vmx.pi_desc.control |
> -  (1 << POSTED_INTR_ON);
> +do {
> +/*
> + * Currently, we don't support urgent interrupt, all
> + * interrupts are recognized as non-urgent interrupt,
> + * Besides that, if 'ON' is already set, no need to
> + * sent posted-interrupts notification event as well,
> + * according to hardware behavior.
> + */

Would be nice to s/sent/send/ here as you move it (maybe also
remove the plural from "posted-interrupts") and - if possible -
re-flow for the now increased space on the right side.

Jan

Re: [RESEND][PATCH v2 1/7] xen/guest_access: Add emacs magics

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> Add emacs magics for xen/guest_access.h and
> asm-x86/guest_access.h.
> 
> Signed-off-by: Julien Grall 
> Acked-by: Jan Beulich 

Most of file in Xen source code seem to have a white line before the “emacs 
magics”.
If this is something that should be enforced, it should be done here.

If not the change seems ok :-)

> 
> ---
>Changes in v2:
>- Remove the word "missing"
> ---
> xen/include/asm-x86/guest_access.h | 8 
> xen/include/xen/guest_access.h | 8 
> 2 files changed, 16 insertions(+)
> 
> diff --git a/xen/include/asm-x86/guest_access.h 
> b/xen/include/asm-x86/guest_access.h
> index 2be3577bd340..3ffde205f6a1 100644
> --- a/xen/include/asm-x86/guest_access.h
> +++ b/xen/include/asm-x86/guest_access.h
> @@ -160,3 +160,11 @@
> })
> 
> #endif /* __ASM_X86_GUEST_ACCESS_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index 09989df819ce..ef9aaa3efcfe 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -33,3 +33,11 @@ char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) 
> u_buf,
>   size_t size, size_t max_size);
> 
> #endif /* __XEN_GUEST_ACCESS_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.17.1
> 
> 

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
variable"):
> On 29.06.2020 14:05, Ian Jackson wrote:
> > Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure 
> > variable"):
> >> On 26.06.2020 19:00, Andrew Cooper wrote:
> >> ... this may or may not take effect on the file system the sources
> >> are stored on.
> > 
> > In what circumstances might this not take effect ?
> 
> When the file system is incapable of recording execute permissions?
> It has been a common workaround for this in various projects that
> I've worked with to use $(SHELL) to account for that, so the actual
> permissions from the fs don't matter. (There may be mount options
> to make everything executable on such file systems, but people may
> be hesitant to use them.)

I don't think we support building from sources which have been
unpacked onto such filesystems.  Other projects which might actually
need to build on Windows or something do do this $(SHELL) thing or an
equivalent, but I don't think that's us.

But obviously that opinion of mine is not a blocker for Andy's patch
since Andy is going in the right direction, regardless.

Andrew Cooper writes ("[PATCH v2] tools/configure: drop BASH configure 
variable"):
> This is a weird variable to have in the first place.  The only user of it is
> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> run with this are shebang sh anyway, so don't need bash in the first place.
> 
> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> CONFIG_SHELL, and drop the $BASH variable to prevent further use.

In response to this commit message, I wrote:

> > Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure 
> > variable"):
> > Thanks for this cleanup.  I agree with the basic idea.
> >
> > However, did you run these scripts with dash, or review them, to check
> > for bashisms ?

And you replied:

> Yes, to all of the above.
> 
> They are both very thin wrappers (doing some argument shuffling) around
> large AWK scripts.

Can you please put this information in the commit message where it
belongs ?  As a rule we should know in future what we were thinking
when a change was made, and as I say "are shebang sh anyway, so don't
need bash in the first place" is weak evidence.

With that change,

Acked-by: Ian Jackson 

Thanks,
Ian.

Re: [PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Jan Beulich]

On 31.07.2020 14:39, Paul Durrant wrote:
> From: Paul Durrant 
> 
> Re-factor the code to take advantage of the fact that the APIC access page is
> a 'special' page.

Hmm, that's going quite as far as I was thinking to go: In
particular, you leave in place the set_mmio_p2m_entry() use
in vmx_alloc_vlapic_mapping(). With that replaced, the
re-ordering in epte_get_entry_emt() that you do shouldn't
be necessary; you'd simple drop the checking of the
specific MFN. However, ...

> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>  return -1;
>  }
>  
> -if ( direct_mmio )
> -{
> -if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
> -return MTRR_TYPE_UNCACHABLE;
> -if ( order )
> -return -1;

... this part of the logic wants retaining, I think, i.e.
reporting back to the guest that the mapping needs splitting.
I'm afraid I have to withdraw my R-b on patch 1 for this
reason, as the check needs to be added there already.

Jan

Re: [RESEND][PATCH v2 2/7] xen/arm: kernel: Re-order the includes

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
> 
> Signed-off-by: Julien Grall 
Reviewed-by: Bertrand Marquis 

> ---
> xen/arch/arm/kernel.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 8eff0748367d..f95fa392af44 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -3,20 +3,20 @@
>  *
>  * Copyright (C) 2011 Citrix Systems, Inc.
>  */
> +#include 
> #include 
> +#include 
> #include 
> #include 
> +#include 
> #include 
> -#include 
> #include 
> -#include 
> -#include 
> -#include 
> -#include 
> #include 
> 
> +#include 
> #include 
> #include 
> +#include 
> 
> #define UIMAGE_MAGIC  0x27051956
> #define UIMAGE_NMLEN  32
> -- 
> 2.17.1
> 
> 

Re: [RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Bertrand Marquis]

> On 31 Jul 2020, at 14:53, Bertrand Marquis  wrote:
> 
> 
> 
>> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
>> 
>> From: Julien Grall 
>> 
>> We usually have xen/ includes first and then asm/. They are also ordered
>> alphabetically among themselves.
>> 
>> Signed-off-by: Julien Grall 
> 
> This could have been merged in patch 3.
> 
> But anyway:
> Reviewed-by: Bertrand Marquis 
> 
> 
>> ---
>> xen/arch/arm/guestcopy.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
>> index 7a0f3e9d5fc6..c8023e2bca5d 100644
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -1,7 +1,8 @@
>> -#include 
>> #include 
>> +#include 
>> #include 
>> #include 
>> +
>> #include 
>> #include 
>> 
>> --
>> 2.17.1
>> 
>> 
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended 
> recipient, please notify the sender immediately and do not disclose the 
> contents to any other person, use it for any purpose, or store or copy the 
> information in any medium. Thank you.

sorry for the notice, i need to find a way to turn it off automatically :-)

Re: [RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
>
> From: Julien Grall 
>
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
>
> Signed-off-by: Julien Grall 

This could have been merged in patch 3.

But anyway:
Reviewed-by: Bertrand Marquis 


> ---
> xen/arch/arm/guestcopy.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7a0f3e9d5fc6..c8023e2bca5d 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,7 +1,8 @@
> -#include 
> #include 
> +#include 
> #include 
> #include 
> +
> #include 
> #include 
>
> --
> 2.17.1
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.

[PATCH 1/6] xen/gntdev: Fix dmabuf import with non-zero sgt offset

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

It is possible that the scatter-gather table during dmabuf import has
non-zero offset of the data, but user-space doesn't expect that.
Fix this by failing the import, so user-space doesn't access wrong data.

Fixes: 37ccb44d0b00 ("xen/gntdev: Implement dma-buf import functionality")

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/xen/gntdev-dmabuf.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 75d3bb948bf3..b1b6eebafd5d 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -613,6 +613,14 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct 
device *dev,
goto fail_detach;
}
 
+   /* Check that we have zero offset. */
+   if (sgt->sgl->offset) {
+   ret = ERR_PTR(-EINVAL);
+   pr_debug("DMA buffer has %d bytes offset, user-space expects 
0\n",
+sgt->sgl->offset);
+   goto fail_unmap;
+   }
+
/* Check number of pages that imported buffer has. */
if (attach->dmabuf->size != gntdev_dmabuf->nr_pages << PAGE_SHIFT) {
ret = ERR_PTR(-EINVAL);
-- 
2.17.1

[PATCH 5/6] drm/xen-front: Pass dumb buffer data offset to the backend

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

While importing a dmabuf it is possible that the data of the buffer
is put with offset which is indicated by the SGT offset.
Respect the offset value and forward it to the backend.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 6 --
 drivers/gpu/drm/xen/xen_drm_front.h | 2 +-
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 88db2726e8ce..013c9e0e412c 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -157,7 +157,8 @@ int xen_drm_front_mode_set(struct 
xen_drm_front_drm_pipeline *pipeline,
 
 int xen_drm_front_dbuf_create(struct xen_drm_front_info *front_info,
  u64 dbuf_cookie, u32 width, u32 height,
- u32 bpp, u64 size, struct page **pages)
+ u32 bpp, u64 size, u32 offset,
+ struct page **pages)
 {
struct xen_drm_front_evtchnl *evtchnl;
struct xen_drm_front_dbuf *dbuf;
@@ -194,6 +195,7 @@ int xen_drm_front_dbuf_create(struct xen_drm_front_info 
*front_info,
req->op.dbuf_create.gref_directory =
xen_front_pgdir_shbuf_get_dir_start(&dbuf->shbuf);
req->op.dbuf_create.buffer_sz = size;
+   req->op.dbuf_create.data_ofs = offset;
req->op.dbuf_create.dbuf_cookie = dbuf_cookie;
req->op.dbuf_create.width = width;
req->op.dbuf_create.height = height;
@@ -408,7 +410,7 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
ret = xen_drm_front_dbuf_create(drm_info->front_info,
xen_drm_front_dbuf_to_cookie(obj),
args->width, args->height, args->bpp,
-   args->size,
+   args->size, 0,
xen_drm_front_gem_get_pages(obj));
if (ret)
goto fail_backend;
diff --git a/drivers/gpu/drm/xen/xen_drm_front.h 
b/drivers/gpu/drm/xen/xen_drm_front.h
index f92c258350ca..54486d89650e 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -145,7 +145,7 @@ int xen_drm_front_mode_set(struct 
xen_drm_front_drm_pipeline *pipeline,
 
 int xen_drm_front_dbuf_create(struct xen_drm_front_info *front_info,
  u64 dbuf_cookie, u32 width, u32 height,
- u32 bpp, u64 size, struct page **pages);
+ u32 bpp, u64 size, u32 offset, struct page 
**pages);
 
 int xen_drm_front_fb_attach(struct xen_drm_front_info *front_info,
u64 dbuf_cookie, u64 fb_cookie, u32 width,
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 4ec8a49241e1..39ff95b75357 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -210,7 +210,8 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 
ret = xen_drm_front_dbuf_create(drm_info->front_info,

xen_drm_front_dbuf_to_cookie(&xen_obj->base),
-   0, 0, 0, size, xen_obj->pages);
+   0, 0, 0, size, sgt->sgl->offset,
+   xen_obj->pages);
if (ret < 0)
return ERR_PTR(ret);
 
-- 
2.17.1

[PATCH 6/6] drm/xen-front: Add support for EDID based configuration

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
request which allows frontends to request EDID structure per
connector. This request is optional and if not supported by the
backend then visible area is still defined by the relevant
XenStore's "resolution" property.
If backend provides EDID with XENDISPL_OP_GET_EDID request then
its values must take precedence over the resolutions defined in
XenStore.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 62 
 drivers/gpu/drm/xen/xen_drm_front.h |  9 ++-
 drivers/gpu/drm/xen/xen_drm_front_cfg.c | 82 +
 drivers/gpu/drm/xen/xen_drm_front_cfg.h |  7 ++
 drivers/gpu/drm/xen/xen_drm_front_conn.c| 26 ++-
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c |  3 +
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.h |  3 +
 drivers/gpu/drm/xen/xen_drm_front_kms.c |  5 ++
 8 files changed, 194 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 013c9e0e412c..cc5981bdbfb3 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -381,6 +381,59 @@ void xen_drm_front_on_frame_done(struct xen_drm_front_info 
*front_info,
fb_cookie);
 }
 
+int xen_drm_front_get_edid(struct xen_drm_front_info *front_info,
+  int conn_idx, struct page **pages,
+  u32 buffer_sz, u32 *edid_sz)
+{
+   struct xen_drm_front_evtchnl *evtchnl;
+   struct xen_front_pgdir_shbuf_cfg buf_cfg;
+   struct xen_front_pgdir_shbuf shbuf;
+   struct xendispl_req *req;
+   unsigned long flags;
+   int ret;
+
+   if (unlikely(conn_idx >= front_info->num_evt_pairs))
+   return -EINVAL;
+
+   memset(&buf_cfg, 0, sizeof(buf_cfg));
+   buf_cfg.xb_dev = front_info->xb_dev;
+   buf_cfg.num_pages = DIV_ROUND_UP(buffer_sz, PAGE_SIZE);
+   buf_cfg.pages = pages;
+   buf_cfg.pgdir = &shbuf;
+   buf_cfg.be_alloc = false;
+
+   ret = xen_front_pgdir_shbuf_alloc(&buf_cfg);
+   if (ret < 0)
+   return ret;
+
+   evtchnl = &front_info->evt_pairs[conn_idx].req;
+
+   mutex_lock(&evtchnl->u.req.req_io_lock);
+
+   spin_lock_irqsave(&front_info->io_lock, flags);
+   req = be_prepare_req(evtchnl, XENDISPL_OP_GET_EDID);
+   req->op.get_edid.gref_directory =
+   xen_front_pgdir_shbuf_get_dir_start(&shbuf);
+   req->op.get_edid.buffer_sz = buffer_sz;
+
+   ret = be_stream_do_io(evtchnl, req);
+   spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+   if (ret < 0)
+   goto fail;
+
+   ret = be_stream_wait_io(evtchnl);
+   if (ret < 0)
+   goto fail;
+
+   *edid_sz = evtchnl->u.req.resp.get_edid.edid_sz;
+
+fail:
+   mutex_unlock(&evtchnl->u.req.req_io_lock);
+   xen_front_pgdir_shbuf_free(&shbuf);
+   return ret;
+}
+
 static int xen_drm_drv_dumb_create(struct drm_file *filp,
   struct drm_device *dev,
   struct drm_mode_create_dumb *args)
@@ -466,6 +519,7 @@ static void xen_drm_drv_release(struct drm_device *dev)
xenbus_switch_state(front_info->xb_dev,
XenbusStateInitialising);
 
+   xen_drm_front_cfg_free(front_info, &front_info->cfg);
kfree(drm_info);
 }
 
@@ -562,6 +616,7 @@ static int xen_drm_drv_init(struct xen_drm_front_info 
*front_info)
drm_mode_config_cleanup(drm_dev);
drm_dev_put(drm_dev);
 fail:
+   xen_drm_front_cfg_free(front_info, &front_info->cfg);
kfree(drm_info);
return ret;
 }
@@ -622,7 +677,14 @@ static int displback_initwait(struct xen_drm_front_info 
*front_info)
 
 static int displback_connect(struct xen_drm_front_info *front_info)
 {
+   int ret;
+
xen_drm_front_evtchnl_set_state(front_info, EVTCHNL_STATE_CONNECTED);
+
+   /* We are all set to read additional configuration from the backend. */
+   ret = xen_drm_front_cfg_tail(front_info, &front_info->cfg);
+   if (ret < 0)
+   return ret;
return xen_drm_drv_init(front_info);
 }
 
diff --git a/drivers/gpu/drm/xen/xen_drm_front.h 
b/drivers/gpu/drm/xen/xen_drm_front.h
index 54486d89650e..be0c982f4d82 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -112,9 +112,12 @@ struct xen_drm_front_drm_pipeline {
struct drm_simple_display_pipe pipe;
 
struct drm_connector conn;
-   /* These are only for connector mode checking */
+   /* These are only for connector mode checking if no EDID present */
int width, height;
 
+   /* Is not NULL if EDID is used for connector configuration. */
+   struct edid *edid;
+
struct drm_pending_vblank_event *pending_event;
 
str

[PATCH 3/6] drm/xen-front: Add YUYV to supported formats

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

Add YUYV to supported formats, so the frontend can work with the
formats used by cameras and other HW.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/gpu/drm/xen/xen_drm_front_conn.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_conn.c 
b/drivers/gpu/drm/xen/xen_drm_front_conn.c
index 459702fa990e..44f1f70c0aed 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_conn.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_conn.c
@@ -33,6 +33,7 @@ static const u32 plane_formats[] = {
DRM_FORMAT_ARGB,
DRM_FORMAT_XRGB1555,
DRM_FORMAT_ARGB1555,
+   DRM_FORMAT_YUYV,
 };
 
 const u32 *xen_drm_front_conn_get_formats(int *format_count)
-- 
2.17.1

[PATCH 4/6] xen: Sync up with the canonical protocol definition in Xen

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

This is the sync up with the canonical definition of the
display protocol in Xen.

1. Add protocol version as an integer

Version string, which is in fact an integer, is hard to handle in the
code that supports different protocol versions. To simplify that
also add the version as an integer.

2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE

There are cases when display data buffer is created with non-zero
offset to the data start. Handle such cases and provide that offset
while creating a display buffer.

3. Add XENDISPL_OP_GET_EDID command

Add an optional request for reading Extended Display Identification
Data (EDID) structure which allows better configuration of the
display connectors over the configuration set in XenStore.
With this change connectors may have multiple resolutions defined
with respect to detailed timing definitions and additional properties
normally provided by displays.

If this request is not supported by the backend then visible area
is defined by the relevant XenStore's "resolution" property.

If backend provides extended display identification data (EDID) with
XENDISPL_OP_GET_EDID request then EDID values must take precedence
over the resolutions defined in XenStore.

4. Bump protocol version to 2.

Signed-off-by: Oleksandr Andrushchenko 
---
 include/xen/interface/io/displif.h | 91 +-
 1 file changed, 88 insertions(+), 3 deletions(-)

diff --git a/include/xen/interface/io/displif.h 
b/include/xen/interface/io/displif.h
index fdc279dc4a88..c2d900186883 100644
--- a/include/xen/interface/io/displif.h
+++ b/include/xen/interface/io/displif.h
@@ -38,7 +38,8 @@
  *   Protocol version
  **
  */
-#define XENDISPL_PROTOCOL_VERSION  "1"
+#define XENDISPL_PROTOCOL_VERSION  "2"
+#define XENDISPL_PROTOCOL_VERSION_INT   2
 
 /*
  **
@@ -202,6 +203,9 @@
  *  Width and height of the connector in pixels separated by
  *  XENDISPL_RESOLUTION_SEPARATOR. This defines visible area of the
  *  display.
+ *  If backend provides extended display identification data (EDID) with
+ *  XENDISPL_OP_GET_EDID request then EDID values must take precedence
+ *  over the resolutions defined here.
  *
  *-- Connector Request Transport Parameters ---
  *
@@ -349,6 +353,8 @@
 #define XENDISPL_OP_FB_DETACH  0x13
 #define XENDISPL_OP_SET_CONFIG 0x14
 #define XENDISPL_OP_PG_FLIP0x15
+/* The below command is available in protocol version 2 and above. */
+#define XENDISPL_OP_GET_EDID   0x16
 
 /*
  **
@@ -377,6 +383,10 @@
 #define XENDISPL_FIELD_BE_ALLOC"be-alloc"
 #define XENDISPL_FIELD_UNIQUE_ID   "unique-id"
 
+#define XENDISPL_EDID_BLOCK_SIZE   128
+#define XENDISPL_EDID_BLOCK_COUNT  256
+#define XENDISPL_EDID_MAX_SIZE (XENDISPL_EDID_BLOCK_SIZE * 
XENDISPL_EDID_BLOCK_COUNT)
+
 /*
  **
  *  STATUS RETURN CODES
@@ -451,7 +461,9 @@
  * +++++
  * |   gref_directory  | 40
  * +++++
- * | reserved  | 44
+ * | data_ofs  | 44
+ * +++++
+ * | reserved  | 48
  * +++++
  * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
  * +++++
@@ -494,6 +506,7 @@
  *   buffer size (buffer_sz) exceeds what can be addressed by this single page,
  *   then reference to the next page must be supplied (see gref_dir_next_page
  *   below)
+ * data_ofs - uint32_t, offset of the data in the buffer, octets
  */
 
 #define XENDISPL_DBUF_FLG_REQ_ALLOC(1 << 0)
@@ -506,6 +519,7 @@ struct xendispl_dbuf_create_req {
uint32_t buffer_sz;
uint32_t flags;
grant_ref_t gref_directory;
+   uint32_t data_ofs;
 };
 
 /*
@@ -731,6 +745,44 @@ struct xendispl_page_flip_req {
uint64_t fb_cookie;
 };
 
+/*
+ * Request EDID - request EDID describing current connector:
+ * 01 2   3octet
+ * +++++
+ * |   id| _OP_GET_EDID   |   reserved | 4
+ * +++-

[PATCH 0/6] Fixes and improvements for Xen pvdrm

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

Hello,

This series contains an assorted set of fixes and improvements for
the Xen para-virtualized display driver and grant device driver which
I have collected over the last couple of months:

1. Minor fixes to grant device driver and drm/xen-front.

2. New format (YUYV) added to the list of the PV DRM supported formats
which allows the driver to be used in zero-copying use-cases when
a camera device is the source of the dma-bufs.

3. Synchronization with the latest para-virtualized protocol definition
in Xen [1].

4. SGT offset is now propagated to the backend: while importing a dmabuf
it is possible that the data of the buffer is put with offset which is
indicated by the SGT offset. This is needed for some GPUs which have
non-zero offset.

5. Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
request which allows frontends to request EDID structure per
connector. This request is optional and if not supported by the
backend then visible area is still defined by the relevant
XenStore's "resolution" property.
If backend provides EDID with XENDISPL_OP_GET_EDID request then
its values must take precedence over the resolutions defined in
XenStore.

Thank you,
Oleksandr Andrushchenko

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0

Oleksandr Andrushchenko (6):
  xen/gntdev: Fix dmabuf import with non-zero sgt offset
  drm/xen-front: Fix misused IS_ERR_OR_NULL checks
  drm/xen-front: Add YUYV to supported formats
  xen: Sync up with the canonical protocol definition in Xen
  drm/xen-front: Pass dumb buffer data offset to the backend
  drm/xen-front: Add support for EDID based configuration

 drivers/gpu/drm/xen/xen_drm_front.c | 72 +++-
 drivers/gpu/drm/xen/xen_drm_front.h | 11 ++-
 drivers/gpu/drm/xen/xen_drm_front_cfg.c | 82 +++
 drivers/gpu/drm/xen/xen_drm_front_cfg.h |  7 ++
 drivers/gpu/drm/xen/xen_drm_front_conn.c| 27 +-
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c |  3 +
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.h |  3 +
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 11 +--
 drivers/gpu/drm/xen/xen_drm_front_kms.c |  7 +-
 drivers/xen/gntdev-dmabuf.c |  8 ++
 include/xen/interface/io/displif.h  | 91 -
 11 files changed, 305 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 2/6] drm/xen-front: Fix misused IS_ERR_OR_NULL checks

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko 

The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
display frontend" from Apr 3, 2018, leads to the following static
checker warning:

drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
warn: passing zero to 'ERR_CAST'

drivers/gpu/drm/xen/xen_drm_front_gem.c
   133  struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
   134  size_t size)
   135  {
   136  struct xen_gem_object *xen_obj;
   137
   138  xen_obj = gem_create(dev, size);
   139  if (IS_ERR_OR_NULL(xen_obj))
   140  return ERR_CAST(xen_obj);

Fix this and the rest of misused places with IS_ERR_OR_NULL in the
driver.

Fixes:  c575b7eeb89f: "drm/xen-front: Add support for Xen PV display frontend"

Signed-off-by: Oleksandr Andrushchenko 
Reported-by: Dan Carpenter 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 4 ++--
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 8 
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 3e660fb111b3..88db2726e8ce 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -400,8 +400,8 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
args->size = args->pitch * args->height;
 
obj = xen_drm_front_gem_create(dev, args->size);
-   if (IS_ERR_OR_NULL(obj)) {
-   ret = PTR_ERR_OR_ZERO(obj);
+   if (IS_ERR(obj)) {
+   ret = PTR_ERR(obj);
goto fail;
}
 
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index f0b85e094111..4ec8a49241e1 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -83,7 +83,7 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
 
size = round_up(size, PAGE_SIZE);
xen_obj = gem_create_obj(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return xen_obj;
 
if (drm_info->front_info->cfg.be_alloc) {
@@ -117,7 +117,7 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
 */
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(&xen_obj->base);
-   if (IS_ERR_OR_NULL(xen_obj->pages)) {
+   if (IS_ERR(xen_obj->pages)) {
ret = PTR_ERR(xen_obj->pages);
xen_obj->pages = NULL;
goto fail;
@@ -136,7 +136,7 @@ struct drm_gem_object *xen_drm_front_gem_create(struct 
drm_device *dev,
struct xen_gem_object *xen_obj;
 
xen_obj = gem_create(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return ERR_CAST(xen_obj);
 
return &xen_obj->base;
@@ -194,7 +194,7 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 
size = attach->dmabuf->size;
xen_obj = gem_create_obj(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return ERR_CAST(xen_obj);
 
ret = gem_alloc_pages_array(xen_obj, size);
diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c 
b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 78096bbcd226..ef11b1e4de39 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -60,7 +60,7 @@ fb_create(struct drm_device *dev, struct drm_file *filp,
int ret;
 
fb = drm_gem_fb_create_with_funcs(dev, filp, mode_cmd, &fb_funcs);
-   if (IS_ERR_OR_NULL(fb))
+   if (IS_ERR(fb))
return fb;
 
gem_obj = fb->obj[0];
-- 
2.17.1

Re: [PATCH] xen/arm: cmpxchg: Add missing memory barriers in __cmpxchg_mb_timeout()

[Bertrand Marquis]

> On 30 Jul 2020, at 19:07, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> The function __cmpxchg_mb_timeout() was intended to have the same
> semantics as __cmpxchg_mb(). Unfortunately, the memory barriers were
> not added when first implemented.
> 
> There is no known issue with the existing callers, but the barriers are
> added given this is the expected semantics in Xen.
> 
> The issue was introduced by XSA-295.
> 
> Backport: 4.8+
> Fixes: 86b0bc958373 ("xen/arm: cmpxchg: Provide a new helper that can 
> timeout")
> Signed-off-by: Julien Grall 
Reviewed-by: Bertrand Marquis 

> ---
> xen/include/asm-arm/arm32/cmpxchg.h | 8 +++-
> xen/include/asm-arm/arm64/cmpxchg.h | 8 +++-
> 2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/arm32/cmpxchg.h 
> b/xen/include/asm-arm/arm32/cmpxchg.h
> index 49ca2a0d7ab1..0770f272ee99 100644
> --- a/xen/include/asm-arm/arm32/cmpxchg.h
> +++ b/xen/include/asm-arm/arm32/cmpxchg.h
> @@ -147,7 +147,13 @@ static always_inline bool __cmpxchg_mb_timeout(volatile 
> void *ptr,
>  int size,
>  unsigned int max_try)
> {
> - return __int_cmpxchg(ptr, old, new, size, true, max_try);
> + bool ret;
> +
> + smp_mb();
> + ret = __int_cmpxchg(ptr, old, new, size, true, max_try);
> + smp_mb();
> +
> + return ret;
> }
> 
> #define cmpxchg(ptr,o,n)  \
> diff --git a/xen/include/asm-arm/arm64/cmpxchg.h 
> b/xen/include/asm-arm/arm64/cmpxchg.h
> index 5bc2e1f78674..fc5c60f0bd74 100644
> --- a/xen/include/asm-arm/arm64/cmpxchg.h
> +++ b/xen/include/asm-arm/arm64/cmpxchg.h
> @@ -160,7 +160,13 @@ static always_inline bool __cmpxchg_mb_timeout(volatile 
> void *ptr,
>  int size,
>  unsigned int max_try)
> {
> - return __int_cmpxchg(ptr, old, new, size, true, max_try);
> + bool ret;
> +
> + smp_mb();
> + ret = __int_cmpxchg(ptr, old, new, size, true, max_try);
> + smp_mb();
> +
> + return ret;
> }
> 
> #define cmpxchg(ptr, o, n) \
> -- 
> 2.17.1
> 
> 

Re: kernel-doc and xen.git

[George Dunlap]

> On Jul 31, 2020, at 12:29 PM, Jan Beulich  wrote:
> 
> On 30.07.2020 03:27, Stefano Stabellini wrote:
>> Hi all,
>> 
>> I would like to ask for your feedback on the adoption of the kernel-doc
>> format for in-code comments.
>> 
>> In the FuSa SIG we have started looking into FuSa documents for Xen. One
>> of the things we are investigating are ways to link these documents to
>> in-code comments in xen.git and vice versa.
>> 
>> In this context, Andrew Cooper suggested to have a look at "kernel-doc"
>> [1] during one of the virtual beer sessions at the last Xen Summit.
>> 
>> I did give a look at kernel-doc and it is very promising. kernel-doc is
>> a script that can generate nice rst text documents from in-code
>> comments. (The generated rst files can then be used as input for sphinx
>> to generate html docs.) The comment syntax [2] is simple and similar to
>> Doxygen:
>> 
>>/**
>> * function_name() - Brief description of function.
>> * @arg1: Describe the first argument.
>> * @arg2: Describe the second argument.
>> *One can provide multiple line descriptions
>> *for arguments.
>> */
>> 
>> kernel-doc is actually better than Doxygen because it is a much simpler
>> tool, one we could customize to our needs and with predictable output.
>> Specifically, we could add the tagging, numbering, and referencing
>> required by FuSa requirement documents.
>> 
>> I would like your feedback on whether it would be good to start
>> converting xen.git in-code comments to the kernel-doc format so that
>> proper documents can be generated out of them. One day we could import
>> kernel-doc into xen.git/scripts and use it to generate a set of html
>> documents via sphinx.
> 
> How far is this intended to go? The example is description of a
> function's parameters, which is definitely fine (albeit I wonder
> if there's a hidden implication then that _all_ functions
> whatsoever are supposed to gain such comments). But the text just
> says much more generally "in-code comments", which could mean all
> of them. I'd consider the latter as likely going too far.

I took him to mean comments in the code at the moment, which describe some 
interface, but aren’t in kernel-doc format.  Naturally we wouldn’t want *all* 
comments to be stuffed into a document somewhere.

 -George

Re: kernel-doc and xen.git

[Bertrand Marquis]

> On 31 Jul 2020, at 13:29, Jan Beulich  wrote:
> 
> On 30.07.2020 03:27, Stefano Stabellini wrote:
>> Hi all,
>> 
>> I would like to ask for your feedback on the adoption of the kernel-doc
>> format for in-code comments.
>> 
>> In the FuSa SIG we have started looking into FuSa documents for Xen. One
>> of the things we are investigating are ways to link these documents to
>> in-code comments in xen.git and vice versa.
>> 
>> In this context, Andrew Cooper suggested to have a look at "kernel-doc"
>> [1] during one of the virtual beer sessions at the last Xen Summit.
>> 
>> I did give a look at kernel-doc and it is very promising. kernel-doc is
>> a script that can generate nice rst text documents from in-code
>> comments. (The generated rst files can then be used as input for sphinx
>> to generate html docs.) The comment syntax [2] is simple and similar to
>> Doxygen:
>> 
>>/**
>> * function_name() - Brief description of function.
>> * @arg1: Describe the first argument.
>> * @arg2: Describe the second argument.
>> *One can provide multiple line descriptions
>> *for arguments.
>> */
>> 
>> kernel-doc is actually better than Doxygen because it is a much simpler
>> tool, one we could customize to our needs and with predictable output.
>> Specifically, we could add the tagging, numbering, and referencing
>> required by FuSa requirement documents.
>> 
>> I would like your feedback on whether it would be good to start
>> converting xen.git in-code comments to the kernel-doc format so that
>> proper documents can be generated out of them. One day we could import
>> kernel-doc into xen.git/scripts and use it to generate a set of html
>> documents via sphinx.
> 
> How far is this intended to go? The example is description of a
> function's parameters, which is definitely fine (albeit I wonder
> if there's a hidden implication then that _all_ functions
> whatsoever are supposed to gain such comments). But the text just
> says much more generally "in-code comments", which could mean all
> of them. I'd consider the latter as likely going too far.

The idea in the FuSa project is more to start with external interfaces like 
hypercall definitions and public headers elements.

Bertrand

> 
> Jan

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
>* Add space before and after operator
>* Align \
>* Format comments
> 
> No functional changes expected.
> 
> Signed-off-by: Julien Grall 
Reviewed-by: Bertrand Marquis 

> ---
> xen/include/xen/guest_access.h | 36 +++---
> 1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index 4957b8d1f2b8..52fc7a063249 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -18,20 +18,24 @@
> #define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
> #define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
> 
> -/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> - * to the specified type of XEN_GUEST_HANDLE_PARAM. */
> +/*
> + * Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> + * to the specified type of XEN_GUEST_HANDLE_PARAM.
> + */
> #define guest_handle_cast(hnd, type) ({ \
> type *_x = (hnd).p; \
> -(XEN_GUEST_HANDLE_PARAM(type)) { _x };\
> +(XEN_GUEST_HANDLE_PARAM(type)) { _x };  \
> })
> 
> /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> #define guest_handle_to_param(hnd, type) ({  \
> typeof((hnd).p) _x = (hnd).p;\
> XEN_GUEST_HANDLE_PARAM(type) _y = { _x };\
> -/* type checking: make sure that the pointers inside \
> +/*   \
> + * type checking: make sure that the pointers inside \
>  * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of\
> - * the same type, then return hnd */ \
> + * the same type, then return hnd.   \
> + */  \
> (void)(&_x == &_y.p);\
> _y;  \
> })
> @@ -106,13 +110,13 @@
>  * guest_handle_subrange_okay().
>  */
> 
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({\
> -const typeof(*(ptr)) *_s = (ptr);   \
> -char (*_d)[sizeof(*_s)] = (void *)(hnd).p;  \
> -/* Check that the handle is not for a const type */ \
> -void *__maybe_unused _t = (hnd).p;  \
> -(void)((hnd).p == _s);  \
> -__raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> +#define __copy_to_guest_offset(hnd, off, ptr, nr) ({\
> +const typeof(*(ptr)) *_s = (ptr);   \
> +char (*_d)[sizeof(*_s)] = (void *)(hnd).p;  \
> +/* Check that the handle is not for a const type */ \
> +void *__maybe_unused _t = (hnd).p;  \
> +(void)((hnd).p == _s);  \
> +__raw_copy_to_guest(_d + (off), _s, sizeof(*_s) * (nr));\
> })
> 
> #define __clear_guest_offset(hnd, off, nr) ({\
> @@ -120,10 +124,10 @@
> __raw_clear_guest(_d + (off), nr);   \
> })
> 
> -#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> -const typeof(*(ptr)) *_s = (hnd).p; \
> -typeof(*(ptr)) *_d = (ptr); \
> -__raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> +#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> +const typeof(*(ptr)) *_s = (hnd).p; \
> +typeof(*(ptr)) *_d = (ptr); \
> +__raw_copy_from_guest(_d, _s + (off), sizeof (*_d) * (nr)); \
> })
> 
> #define __copy_field_to_guest(hnd, ptr, field) ({   \
> -- 
> 2.17.1
> 
> 

Re: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> Only a few places are actually including asm/guest_access.h. While this
> is fine today, a follow-up patch will want to move most of the helpers
> from asm/guest_access.h to xen/guest_access.h.
> 
> To prepare the move, everyone should include xen/guest_access.h rather
> than asm/guest_access.h.
> 
> Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
> inclusion is now removed as no-one but the latter should include the
> former.
> 
> Signed-off-by: Julien Grall 
Reviewed-by: Bertrand Marquis 

(sorry forgot to remove the disclaimer in the previous one)
> 
> ---
>Changes in v2:
>- Remove some changes that weren't meant to be here.
> ---
> xen/arch/arm/decode.c| 2 +-
> xen/arch/arm/domain.c| 2 +-
> xen/arch/arm/guest_walk.c| 3 ++-
> xen/arch/arm/guestcopy.c | 2 +-
> xen/arch/arm/vgic-v3-its.c   | 2 +-
> xen/arch/x86/hvm/svm/svm.c   | 2 +-
> xen/arch/x86/hvm/viridian/viridian.c | 2 +-
> xen/arch/x86/hvm/vmx/vmx.c   | 2 +-
> xen/common/libelf/libelf-loader.c| 2 +-
> xen/include/asm-arm/guest_access.h   | 1 -
> xen/lib/x86/private.h| 2 +-
> 11 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 144793c8cea0..792c2e92a7eb 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,12 +17,12 @@
>  * GNU General Public License for more details.
>  */
> 
> +#include 
> #include 
> #include 
> #include 
> 
> #include 
> -#include 
> 
> #include "decode.h"
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 31169326b2e3..9258f6d3faa2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -12,6 +12,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -26,7 +27,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a1cdd7f4afea..b4496c4c86c6 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,8 +16,9 @@
>  */
> 
> #include 
> +#include 
> #include 
> -#include 
> +
> #include 
> #include 
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index c8023e2bca5d..32681606d8fc 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,10 @@
> #include 
> +#include 
> #include 
> #include 
> #include 
> 
> #include 
> -#include 
> 
> #define COPY_flush_dcache   (1U << 0)
> #define COPY_from_guest (0U << 1)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 6e153c698d56..58d939b85f92 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -32,6 +32,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -39,7 +40,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index ca3bbfcbb355..7301f3cd6004 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -16,6 +16,7 @@
>  * this program; If not, see .
>  */
> 
> +#include 
> #include 
> #include 
> #include 
> @@ -34,7 +35,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c 
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54fad..dc7183a54627 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -5,12 +5,12 @@
>  * Hypervisor Top Level Functional Specification for more information.
>  */
> 
> +#include 
> #include 
> #include 
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index eb54aadfbafb..cb5df1e81c9c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,6 +15,7 @@
>  * this program; If not, see .
>  */
> 
> +#include 
> #include 
> #include 
> #include 
> @@ -31,7 +32,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/common/libelf/libelf-loader.c 
> b/xen/common/libelf/libelf-loader.c
> index 0f468727d04a..629cc0d3e611 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -16,7 +16,7 @@
>  */
> 
> #ifdef __XEN__
> -#include 
> +#include 
> #endif
> 
> #include "libelf-private.h"
> diff --git a/xen/include/asm-arm/guest_access.h 
> b/xen/include/asm-arm/guest_access.h
> index 31b9f03f0015..b9a89c495527 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -1,7 +1,6 @@
> #ifndef __ASM_ARM_GUEST_ACCESS_H

Re: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall  wrote:
>
> From: Julien Grall 
>
> Only a few places are actually including asm/guest_access.h. While this
> is fine today, a follow-up patch will want to move most of the helpers
> from asm/guest_access.h to xen/guest_access.h.
>
> To prepare the move, everyone should include xen/guest_access.h rather
> than asm/guest_access.h.
>
> Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
> inclusion is now removed as no-one but the latter should include the
> former.
>
> Signed-off-by: Julien Grall 
Reviewed-by: Bertrand Marquis 

>
> ---
>Changes in v2:
>- Remove some changes that weren't meant to be here.
> ---
> xen/arch/arm/decode.c| 2 +-
> xen/arch/arm/domain.c| 2 +-
> xen/arch/arm/guest_walk.c| 3 ++-
> xen/arch/arm/guestcopy.c | 2 +-
> xen/arch/arm/vgic-v3-its.c   | 2 +-
> xen/arch/x86/hvm/svm/svm.c   | 2 +-
> xen/arch/x86/hvm/viridian/viridian.c | 2 +-
> xen/arch/x86/hvm/vmx/vmx.c   | 2 +-
> xen/common/libelf/libelf-loader.c| 2 +-
> xen/include/asm-arm/guest_access.h   | 1 -
> xen/lib/x86/private.h| 2 +-
> 11 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 144793c8cea0..792c2e92a7eb 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,12 +17,12 @@
>  * GNU General Public License for more details.
>  */
>
> +#include 
> #include 
> #include 
> #include 
>
> #include 
> -#include 
>
> #include "decode.h"
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 31169326b2e3..9258f6d3faa2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -12,6 +12,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -26,7 +27,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a1cdd7f4afea..b4496c4c86c6 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,8 +16,9 @@
>  */
>
> #include 
> +#include 
> #include 
> -#include 
> +
> #include 
> #include 
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index c8023e2bca5d..32681606d8fc 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,10 @@
> #include 
> +#include 
> #include 
> #include 
> #include 
>
> #include 
> -#include 
>
> #define COPY_flush_dcache   (1U << 0)
> #define COPY_from_guest (0U << 1)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 6e153c698d56..58d939b85f92 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -32,6 +32,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -39,7 +40,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index ca3bbfcbb355..7301f3cd6004 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -16,6 +16,7 @@
>  * this program; If not, see .
>  */
>
> +#include 
> #include 
> #include 
> #include 
> @@ -34,7 +35,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c 
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54fad..dc7183a54627 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -5,12 +5,12 @@
>  * Hypervisor Top Level Functional Specification for more information.
>  */
>
> +#include 
> #include 
> #include 
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index eb54aadfbafb..cb5df1e81c9c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,6 +15,7 @@
>  * this program; If not, see .
>  */
>
> +#include 
> #include 
> #include 
> #include 
> @@ -31,7 +32,6 @@
> #include 
> #include 
> #include 
> -#include 
> #include 
> #include 
> #include 
> diff --git a/xen/common/libelf/libelf-loader.c 
> b/xen/common/libelf/libelf-loader.c
> index 0f468727d04a..629cc0d3e611 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -16,7 +16,7 @@
>  */
>
> #ifdef __XEN__
> -#include 
> +#include 
> #endif
>
> #include "libelf-private.h"
> diff --git a/xen/include/asm-arm/guest_access.h 
> b/xen/include/asm-arm/guest_access.h
> index 31b9f03f0015..b9a89c495527 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -1,7 +1,6 @@
> #ifndef __ASM_ARM_GUEST_ACCESS_H__
> #define __ASM_ARM_GUEST_ACCESS_H__
>
> -#include 
> #include 
> #include 
>

[PATCH v2 1/2] x86/hvm: set 'ipat' in EPT for special pages

[Paul Durrant]

From: Paul Durrant 

All non-MMIO ranges (i.e those not mapping real device MMIO regions) that
map valid MFNs are normally marked MTRR_TYPE_WRBACK and 'ipat' is set. Hence
when PV drivers running in a guest populate the BAR space of the Xen Platform
PCI Device with pages such as the Shared Info page or Grant Table pages,
accesses to these pages will be cachable.

However, should IOMMU mappings be enabled be enabled for the guest then these
accesses become uncachable. This has a substantial negative effect on I/O
throughput of PV devices. Arguably PV drivers should bot be using BAR space to
host the Shared Info and Grant Table pages but it is currently commonplace for
them to do this and so this problem needs mitigation. Hence this patch makes
sure the 'ipat' bit is set for any special page regardless of where in GFN
space it is mapped.

NOTE: Clearly this mitigation only applies to Intel EPT. It is not obvious
  that there is any similar mitigation possible for AMD NPT. Downstreams
  such as Citrix XenServer have been carrying a patch similar to this for
  several releases though.

Signed-off-by: Paul Durrant 
Reviewed-by: Jan Beulich 
---
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 
---
 xen/arch/x86/hvm/mtrr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 511c3be1c8..3ad813ed15 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -830,7 +830,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, 
mfn_t mfn,
 return MTRR_TYPE_UNCACHABLE;
 }
 
-if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
+if ( (!is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
+ is_special_page(mfn_to_page(mfn)) )
 {
 *ipat = 1;
 return MTRR_TYPE_WRBACK;
-- 
2.20.1

Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid

[Jan Beulich]

On 31.07.2020 14:35, Julien Grall wrote:
> Hi Jan,
> 
> On 31/07/2020 10:53, Jan Beulich wrote:
>> On 31.07.2020 10:38, Eslam Elnikety wrote:
>>> On 28.07.20 19:51, Jan Beulich wrote:
 On 28.07.2020 11:26, Andrew Cooper wrote:
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -73,7 +73,13 @@ struct hpet_registers {
>    uint64_t isr;   /* interrupt status reg */
>    uint64_t mc64;  /* main counter */
>    struct {    /* timers */
> -    uint64_t config;    /* configuration/cap */
> +    union {
> +    uint64_t config;    /* configuration/cap */
> +    struct {
> +    uint32_t _;
> +    uint32_t route;
> +    };
> +    };

 So long as there are no static initializers for this construct
 that would then suffer the old-gcc problem, this is of course a
 fine arrangement to make.
>>>
>>> I have to admit that I have no clue what the "old-gcc" problem is. I am
>>> curious, and I would appreciate pointers to figure out if/how to
>>> resolve. Is that an old, existing problem? Or a problem that was present
>>> in older versions of gcc?
>>
>> Well, as already said - the problem is with old gcc not dealing
>> well with initializers of structs/unions with unnamed fields.
> 
> You seem to know quite well the problem. So would you mind to give us 
> more details on which GCC version is believed to be broken?

I don't recall for sure, but iirc anything before 4.4.

Jan

[PATCH v2 2/2] x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

[Paul Durrant]

From: Paul Durrant 

Re-factor the code to take advantage of the fact that the APIC access page is
a 'special' page.

Suggested-by: Jan Beulich 
Signed-off-by: Paul Durrant 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 
---
 xen/arch/x86/hvm/mtrr.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 3ad813ed15..0992f05e8f 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -814,29 +814,22 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
gfn, mfn_t mfn,
 return -1;
 }
 
-if ( direct_mmio )
-{
-if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
-return MTRR_TYPE_UNCACHABLE;
-if ( order )
-return -1;
-*ipat = 1;
-return MTRR_TYPE_WRBACK;
-}
-
 if ( !mfn_valid(mfn) )
 {
 *ipat = 1;
 return MTRR_TYPE_UNCACHABLE;
 }
 
-if ( (!is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
+if ( (!direct_mmio && !is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
  is_special_page(mfn_to_page(mfn)) )
 {
 *ipat = 1;
 return MTRR_TYPE_WRBACK;
 }
 
+if ( direct_mmio )
+return MTRR_TYPE_UNCACHABLE;
+
 gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, _gfn(gfn), order);
 if ( gmtrr_mtype >= 0 )
 {
-- 
2.20.1

[PATCH v2 0/2] epte_get_entry_emt() modifications

[Paul Durrant]

From: Paul Durrant 

This series was originally a singleton (of patch #1)

Paul Durrant (2):
  x86/hvm: set 'ipat' in EPT for special pages
  x86/hvm: simplify 'mmio_direct' check in epte_get_entry_emt()

 xen/arch/x86/hvm/mtrr.c | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)
---
Cc: Andrew Cooper 
Cc: Jan Beulich 
Cc: "Roger Pau Monné" 
Cc: Wei Liu 
-- 
2.20.1

Re: RESCHEDULED Call for agenda items for Community Call, August 13 @ 15:00 UTC

[Jan Beulich]

On 31.07.2020 14:27, George Dunlap wrote:
>> On Jul 31, 2020, at 1:25 PM, Jan Beulich  wrote:
>> On 30.07.2020 17:41, George Dunlap wrote:
 On Jul 30, 2020, at 4:17 PM, George Dunlap  
 wrote:

 Hey all,

 The community call is scheduled for next week, 6 August.  I, however, will 
 be on PTO that week; I propose rescheduling it for the following week, 13 
 August, at the same time.

 The proposed agenda is in ZZZ and you can edit to add items.  
 Alternatively, you can reply to this mail directly.
>>>
>>> Sorry, in all my manual templating I seem to have missed this one.  Here’s 
>>> the URL:
>>>
>>> https://cryptpad.fr/pad/#/3/pad/edit/9c58993a08fe97451f0a5b6c8bb906b1/
>>
>> I get "This link does not give you access to the document". Maybe a
>> permissions problem? I've meant to add a "minimum toolchain versions"
>> topic ...
> 
> Try this one?
> 
> https://cryptpad.fr/pad/#/2/pad/edit/VlLdjiw7iBm0R-efOMyCY+Ks/

Ah yes, this one works. Thanks.

Jan

Re: [PATCH] x86/vhpet: Fix type size in timer_int_route_valid

[Julien Grall]

Hi Jan,

On 31/07/2020 10:53, Jan Beulich wrote:

On 31.07.2020 10:38, Eslam Elnikety wrote:

On 28.07.20 19:51, Jan Beulich wrote:

On 28.07.2020 11:26, Andrew Cooper wrote:

--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -73,7 +73,13 @@ struct hpet_registers {
   uint64_t isr;   /* interrupt status reg */
   uint64_t mc64;  /* main counter */
   struct {    /* timers */
-    uint64_t config;    /* configuration/cap */
+    union {
+    uint64_t config;    /* configuration/cap */
+    struct {
+    uint32_t _;
+    uint32_t route;
+    };
+    };


So long as there are no static initializers for this construct
that would then suffer the old-gcc problem, this is of course a
fine arrangement to make.


I have to admit that I have no clue what the "old-gcc" problem is. I am
curious, and I would appreciate pointers to figure out if/how to
resolve. Is that an old, existing problem? Or a problem that was present
in older versions of gcc?


Well, as already said - the problem is with old gcc not dealing
well with initializers of structs/unions with unnamed fields.


You seem to know quite well the problem. So would you mind to give us 
more details on which GCC version is believed to be broken?





If the latter, is that a gcc version that we still care about?


Until someone makes a (justified) proposal what the new minimum
version(s) ought to be, I'm afraid we still have to care. This
topic came up very recently in another context, and I've proposed
to put it on the agenda of the next community call.


I don't think Eslam was requesting to change the limits. He was just 
asking whether one of the compiler we support is affected.


Cheers,


--
Julien Grall

Re: RESCHEDULED Call for agenda items for Community Call, August 13 @ 15:00 UTC

[George Dunlap]

> On Jul 31, 2020, at 1:25 PM, Jan Beulich  wrote:
> 
> On 30.07.2020 17:41, George Dunlap wrote:
>>> On Jul 30, 2020, at 4:17 PM, George Dunlap  wrote:
>>> 
>>> Hey all,
>>> 
>>> The community call is scheduled for next week, 6 August.  I, however, will 
>>> be on PTO that week; I propose rescheduling it for the following week, 13 
>>> August, at the same time.
>>> 
>>> The proposed agenda is in ZZZ and you can edit to add items.  
>>> Alternatively, you can reply to this mail directly.
>> 
>> Sorry, in all my manual templating I seem to have missed this one.  Here’s 
>> the URL:
>> 
>> https://cryptpad.fr/pad/#/3/pad/edit/9c58993a08fe97451f0a5b6c8bb906b1/
> 
> I get "This link does not give you access to the document". Maybe a
> permissions problem? I've meant to add a "minimum toolchain versions"
> topic ...

Try this one?

https://cryptpad.fr/pad/#/2/pad/edit/VlLdjiw7iBm0R-efOMyCY+Ks/

That sounds like a good topic.

 -George

Re: RESCHEDULED Call for agenda items for Community Call, August 13 @ 15:00 UTC

[Jan Beulich]

On 30.07.2020 17:41, George Dunlap wrote:
>> On Jul 30, 2020, at 4:17 PM, George Dunlap  wrote:
>>
>> Hey all,
>>
>> The community call is scheduled for next week, 6 August.  I, however, will 
>> be on PTO that week; I propose rescheduling it for the following week, 13 
>> August, at the same time.
>>
>> The proposed agenda is in ZZZ and you can edit to add items.  Alternatively, 
>> you can reply to this mail directly.
> 
> Sorry, in all my manual templating I seem to have missed this one.  Here’s 
> the URL:
> 
> https://cryptpad.fr/pad/#/3/pad/edit/9c58993a08fe97451f0a5b6c8bb906b1/

I get "This link does not give you access to the document". Maybe a
permissions problem? I've meant to add a "minimum toolchain versions"
topic ...

Jan

Re: [PATCH v1] tools/xen-cpuid: show enqcmd

[Andrew Cooper]

On 31/07/2020 13:15, Olaf Hering wrote:
> Am Fri, 31 Jul 2020 13:04:35 +0100
> schrieb Andrew Cooper :
>
>> And in particular, probably missing from libxl_cpuid.c, which I was
>> meaning to check when I've got a free moment.
> Will a ever domU see this flag? I just spotted the <29> when comparing 
> 'xen-cpuid' output between recent Xen releases. It shows up just in the 
> 'Known' section at this point.

For future platforms supporting Compute eXpress Link, PCIPassthrough
will be able to give various accelerators to VMs, and the ENQCMD{,S}
instructions is what the guest will use to talk to hardware.

As a set of functionality, think DPDK but with with userspace (or the
guest) able to submit work to hardware directly, through an interface
designed to be safe for this kind of thing.

~Andrew

Re: [PATCH v1] tools/xen-cpuid: show enqcmd

[Jan Beulich]

On 31.07.2020 14:04, Andrew Cooper wrote:
> On 31/07/2020 13:03, Jan Beulich wrote:
>> On 30.07.2020 18:34, Olaf Hering wrote:
>>> Translate <29> into a feature string.
>>>
>>> Signed-off-by: Olaf Hering 
>> Acked-by: Jan Beulich 
>>
>> Albeit I'm pretty sure there are more missing than just this lone one.
> 
> And in particular, probably missing from libxl_cpuid.c, which I was
> meaning to check when I've got a free moment.

As it's not just this one, but e.g. also the two movdir* ones,
I thought I'd not require the other side to also be changed,
the more that enqcmd also doesn't get exposed to guests at all
right now.

Jan

Re: [PATCH v3] xen/arm: Convert runstate address during hypcall

[Jan Beulich]

On 30.07.2020 22:50, Julien Grall wrote:
> On 30/07/2020 11:24, Bertrand Marquis wrote:
>> At the moment on Arm, a Linux guest running with KTPI enabled will
>> cause the following error when a context switch happens in user mode:
>> (XEN) p2m.c:1890: d1v0: Failed to walk page-table va 0xff837ebe0cd0
>>
>> The error is caused by the virtual address for the runstate area
>> registered by the guest only being accessible when the guest is running
>> in kernel space when KPTI is enabled.
>>
>> To solve this issue, this patch is doing the translation from virtual
>> address to physical address during the hypercall and mapping the
>> required pages using vmap. This is removing the conversion from virtual
>> to physical address during the context switch which is solving the
>> problem with KPTI.
> 
> To echo what Jan said on the previous version, this is a change in a 
> stable ABI and therefore may break existing guest. FAOD, I agree in 
> principle with the idea. However, we want to explain why breaking the 
> ABI is the *only* viable solution.
> 
>  From my understanding, it is not possible to fix without an ABI 
> breakage because the hypervisor doesn't know when the guest will switch 
> back from userspace to kernel space.

And there's also no way to know on Arm, by e.g. enabling a suitable
intercept?

> The risk is the information 
> provided by the runstate wouldn't contain accurate information and could 
> affect how the guest handle stolen time.
> 
> Additionally there are a few issues with the current interface:
> 1) It is assuming the virtual address cannot be re-used by the 
> userspace. Thanksfully Linux have a split address space. But this may 
> change with KPTI in place.
> 2) When update the page-tables, the guest has to go through an 
> invalid mapping. So the translation may fail at any point.
> 
> IOW, the existing interface can lead to random memory corruption and 
> inacurracy of the stolen time.
> 
>>
>> This is done only on arm architecture, the behaviour on x86 is not
>> modified by this patch and the address conversion is done as before
>> during each context switch.
>>
>> This is introducing several limitations in comparison to the previous
>> behaviour (on arm only):
>> - if the guest is remapping the area at a different physical address Xen
>> will continue to update the area at the previous physical address. As
>> the area is in kernel space and usually defined as a global variable this
>> is something which is believed not to happen. If this is required by a
>> guest, it will have to call the hypercall with the new area (even if it
>> is at the same virtual address).
>> - the area needs to be mapped during the hypercall. For the same reasons
>> as for the previous case, even if the area is registered for a different
>> vcpu. It is believed that registering an area using a virtual address
>> unmapped is not something done.
> 
> This is not clear whether the virtual address refer to the current vCPU 
> or the vCPU you register the runstate for. From the past discussion, I 
> think you refer to the former. It would be good to clarify.
> 
> Additionally, all the new restrictions should be documented in the 
> public interface. So an OS developper can find the differences between 
> the architectures.
> 
> To answer Jan's concern, we certainly don't know all the guest OSes 
> existing, however we also need to balance the benefit for a large 
> majority of the users.
> 
>  From previous discussion, the current approach was deemed to be 
> acceptable on Arm and, AFAICT, also x86 (see [1]).
> 
> TBH, I would rather see the approach to be common. For that, we would an 
> agreement from Andrew and Jan in the approach here. Meanwhile, I think 
> this is the best approach to address the concern from Arm users.

Just FTR: If x86 was to also change, VCPUOP_register_vcpu_time_memory_area
would need taking care of as well, as it's using the same underlying model
(including recovery logic when, while the guest is in user mode, the
update has failed).

>> @@ -275,36 +276,156 @@ static void ctxt_switch_to(struct vcpu *n)
>>   virt_timer_restore(n);
>>   }
>>   
>> -/* Update per-VCPU guest runstate shared memory area (if registered). */
>> -static void update_runstate_area(struct vcpu *v)
>> +static void cleanup_runstate_vcpu_locked(struct vcpu *v)
>>   {
>> -void __user *guest_handle = NULL;
>> +if ( v->arch.runstate_guest )
>> +{
>> +vunmap((void *)((unsigned long)v->arch.runstate_guest & PAGE_MASK));
>> +
>> +put_page(v->arch.runstate_guest_page[0]);
>> +
>> +if ( v->arch.runstate_guest_page[1] )
>> +put_page(v->arch.runstate_guest_page[1]);
>> +
>> +v->arch.runstate_guest = NULL;
>> +}
>> +}
>> +
>> +void arch_vcpu_cleanup_runstate(struct vcpu *v)
>> +{
>> +spin_lock(&v->arch.runstate_guest_lock);
>> +
>> +cleanup_runstate_vcpu_locked(v);
>> +
>> +spin_unlock(&v->arch.runstate_guest_lock);
>> +}
>> +
>> +static 

Re: [PATCH v1] tools/xen-cpuid: show enqcmd

[Olaf Hering]

Am Fri, 31 Jul 2020 13:04:35 +0100
schrieb Andrew Cooper :

> And in particular, probably missing from libxl_cpuid.c, which I was
> meaning to check when I've got a free moment.

Will a ever domU see this flag? I just spotted the <29> when comparing 
'xen-cpuid' output between recent Xen releases. It shows up just in the 'Known' 
section at this point.


Olaf


pgp9XLi0PMCmx.pgp
Description: Digitale Signatur von OpenPGP

[OSSTEST PATCH v2 18/41] duration_estimator: Move duration query loop into database

[Ian Jackson]

Stuff the two queries together: we use the firsty query as a WITH
clause.  This is significantly faster, perhaps because the query
optimiser does a better job but probably just because it saves on
round trips.

No functional change.

Perf: subjectively this seemed to help when the cache was cold.  Now I
have a warm cache and it doesn't seem to make much difference.

Perf: runtime of my test case now ~5-7s.

Example queries before (from the debugging output):

 Query A part I:

SELECT f.flight AS flight,
   j.job AS job,
   f.started AS started,
   j.status AS status
 FROM flights f
 JOIN jobs j USING (flight)
 JOIN runvars r
 ON  f.flight=r.flight
AND  r.name=?
WHERE  j.job=r.job
  AND  f.blessing=?
  AND  f.branch=?
  AND  j.job=?
  AND  r.val=?
  AND  (j.status='pass' OR j.status='fail'
   OR j.status='truncated'!)
  AND  f.started IS NOT NULL
  AND  f.started >= ?
 ORDER BY f.started DESC

 With bind variables:
 "test-amd64-i386-xl-pvshim"
 "guest-start"

 Query B part I:

SELECT f.flight AS flight,
   s.job AS job,
   NULL as started,
   NULL as status,
   max(s.finished) AS max_finished
  FROM steps s JOIN flights f
ON s.flight=f.flight
 WHERE s.job=? AND f.blessing=? AND f.branch=?
   AND s.finished IS NOT NULL
   AND f.started IS NOT NULL
   AND f.started >= ?
 GROUP BY f.flight, s.job
 ORDER BY max_finished DESC

 With bind variables:
"test-armhf-armhf-libvirt"
'real'
"xen-unstable"
1594144469

 Query common part II:

WITH tsteps AS
(
SELECT *
  FROM steps
 WHERE flight=? AND job=?
)
, tsteps2 AS
(
SELECT *
  FROM tsteps
 WHERE finished <=
 (SELECT finished
FROM tsteps
   WHERE tsteps.testid = ?)
)
SELECT (
SELECT max(finished)-min(started)
  FROM tsteps2
  ) - (
SELECT sum(finished-started)
  FROM tsteps2
 WHERE step = 'ts-hosts-allocate'
  )
AS duration

 With bind variables from previous query, eg:
 152045
 "test-armhf-armhf-libvirt"
 "guest-start.2"

After:

 Query A (combined):

WITH f AS (
SELECT f.flight AS flight,
   j.job AS job,
   f.started AS started,
   j.status AS status
 FROM flights f
 JOIN jobs j USING (flight)
 JOIN runvars r
 ON  f.flight=r.flight
AND  r.name=?
WHERE  j.job=r.job
  AND  f.blessing=?
  AND  f.branch=?
  AND  j.job=?
  AND  r.val=?
  AND  (j.status='pass' OR j.status='fail'
   OR j.status='truncated'!)
  AND  f.started IS NOT NULL
  AND  f.started >= ?
 ORDER BY f.started DESC

)
SELECT flight, max_finished, job, started, status,
(
WITH tsteps AS
(
SELECT *
  FROM steps
 WHERE flight=f.flight AND job=f.job
)
, tsteps2 AS
(
SELECT *
  FROM tsteps
 WHERE finished <=
 (SELECT finished
FROM tsteps
   WHERE tsteps.testid = ?)
)
SELECT (
SELECT max(finished)-min(started)
  FROM tsteps2
  ) - (
SELECT sum(finished-started)
  FROM tsteps2
 WHERE step = 'ts-hosts-allocate'
  )
AS duration

) FROM f

 Query B (combined):

WITH f AS (
SELECT f.flight AS flight,
   s.job AS job,
   NULL as started,
   NULL as status,
   max(s.finished) AS max_finished
  FROM steps s JOIN flights f
ON s.flight=f.flight
 WHERE s.job=? AND f.blessing=? AND f.branch=?
   AND s.finished IS NOT NULL
   AND f.started IS NOT NULL
   AND f.started >= ?
  

[OSSTEST PATCH v2 10/41] sg-report-flight: Use WITH clause to use index for $anypassq

[Ian Jackson]

Perf: runtime of my test case now ~11s

Example query before (from the Perl DBI trace):

SELECT * FROM flights JOIN steps USING (flight)
WHERE (branch='xen-unstable')
  AND job=? and testid=? and status='pass'
  AND ( (TRUE AND flight <= 151903) AND (blessing='real') )
LIMIT 1

After:

WITH s AS
(
SELECT * FROM steps
 WHERE job=? and testid=? and status='pass'
)
SELECT * FROM flights JOIN s USING (flight)
WHERE (branch='xen-unstable')
  AND ( (TRUE AND flight <= 151903) AND (blessing='real') )
LIMIT 1

In both cases with bind vars:

   "test-amd64-i386-xl-pvshim"
   "guest-start"

Diff to the query:

-SELECT * FROM flights JOIN steps USING (flight)
+WITH s AS
+(
+SELECT * FROM steps
+ WHERE job=? and testid=? and status='pass'
+)
+SELECT * FROM flights JOIN s USING (flight)
 WHERE (branch='xen-unstable')
-  AND job=? and testid=? and status='pass'
   AND ( (TRUE AND flight <= 151903) AND (blessing='real') )
 LIMIT 1

Signed-off-by: Ian Jackson 
Reviewed-by: George Dunlap 
---
 schema/steps-job-index.sql |  2 +-
 sg-report-flight   | 14 --
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/schema/steps-job-index.sql b/schema/steps-job-index.sql
index 07dc5a30..2c33af72 100644
--- a/schema/steps-job-index.sql
+++ b/schema/steps-job-index.sql
@@ -1,4 +1,4 @@
--- ##OSSTEST## 006 Preparatory
+-- ##OSSTEST## 006 Needed
 --
 -- This index helps sg-report-flight find if a test ever passed.
 
diff --git a/sg-report-flight b/sg-report-flight
index d06be292..d218b24e 100755
--- a/sg-report-flight
+++ b/sg-report-flight
@@ -849,10 +849,20 @@ sub justifyfailures ($;$) {
 
 my @failures= values %{ $fi->{Failures} };
 
+# In psql 9.6 this WITH clause makes postgresql do the steps query
+# first.  This is good because if this test never passed we can
+# determine that really quickly using the new index, without
+# having to scan the flights table.  (If the test passed we will
+# probably not have to look at many flights to find one, so in
+# that case this is not much worse.)
 my $anypassq= <

[OSSTEST PATCH v2 36/41] cs-bisection-step: Use db_prepare a few times instead of ->do

[Ian Jackson]

With $dbh_tests->do(...), we can only get a debug trace of the queries
by using DBI_TRACE which produces voluminous output.  Using our own
db_prepare invokes our own debugging.

No functional change.

Signed-off-by: Ian Jackson 
---
v2: New patch.
---
 cs-bisection-step | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index ba0c6424..1c165b78 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -197,7 +197,7 @@ END
 END
 };
 
-$dbh_tests->do(do(do(

[OSSTEST PATCH v2 29/41] sg-report-host-history: Drop a redundznt AND clause

[Ian Jackson]

This condition is the same as $flightcond.  (This has no effect on the
db performance since the query planner figures it out, but it is
confusing.)

Signed-off-by: Ian Jackson 
---
 sg-report-host-history | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sg-report-host-history b/sg-report-host-history
index 2ca0e235..f4352fc3 100755
--- a/sg-report-host-history
+++ b/sg-report-host-history
@@ -175,13 +175,12 @@ sub mainquery ($) {
   AND val = ?
   AND $flightcond
AND $restrictflight_cond
-   AND flight > ?
 ORDER BY flight DESC
  LIMIT $limit * 2
 END
 
 print DEBUG "MAINQUERY $host...\n";
-$runvarq->execute($host, $minflight);
+$runvarq->execute($host);
 
 $hosts{$host} = $runvarq->fetchall_arrayref({});
 print DEBUG "MAINQUERY $host got ".(scalar @{ $hosts{$host} })."\n";
-- 
2.20.1

[OSSTEST PATCH v2 22/41] sg-report-host-history: Drop per-job debug etc.

[Ian Jackson]

This printing has a significant effect on the performance of this
program, at least after we optimise various other things.

Signed-off-by: Ian Jackson 
---
 sg-report-host-history | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sg-report-host-history b/sg-report-host-history
index a159df3e..a34458e0 100755
--- a/sg-report-host-history
+++ b/sg-report-host-history
@@ -102,9 +102,9 @@ sub read_existing_logs ($) {
my $k = $1;
s{\%([0-9a-f]{2})}{ chr hex $1 }ge;
$ch->{$k} = $_;
-   print DEBUG "GOTCACHE $hostname $k\n";
+#  print DEBUG "GOTCACHE $hostname $k\n";
}
-   print DEBUG "GOTCACHE $hostname \@ $jr->{flight} $jr->{job} 
$jr->{status},$jr->{name}\n";
+#  print DEBUG "GOTCACHE $hostname \@ $jr->{flight} $jr->{job} 
$jr->{status},$jr->{name}\n";
$tcache->{$jr->{flight},$jr->{job},$jr->{status},$jr->{name}} = $jr;
 }
 close H;
@@ -272,7 +272,7 @@ END
 my @rows;
 my $cachehits = 0;
 foreach my $jr (@$inrows) {
-   print DEBUG "JOB $jr->{flight}.$jr->{job} ";
+   #print DEBUG "JOB $jr->{flight}.$jr->{job} ";
 
my $cacherow =
$tcache->{$jr->{flight},$jr->{job},$jr->{status},$jr->{name}};
@@ -283,11 +283,11 @@ END
 
my $endedrow = jobquery($endedq, $jr, 'e');
if (!$endedrow) {
-   print DEBUG "no-finished\n";
+   #print DEBUG "no-finished\n";
next;
}
-   print DEBUG join " ", map { $endedrow->{$_} } sort keys %$endedrow;
-   print DEBUG ".\n";
+   #print DEBUG join " ", map { $endedrow->{$_} } sort keys %$endedrow;
+   #print DEBUG ".\n";
 
push @rows, { %$jr, %$endedrow };
 }
@@ -329,7 +329,7 @@ END
next;
}
 
-print DEBUG "JR $jr->{flight}.$jr->{job}\n";
+#print DEBUG "JR $jr->{flight}.$jr->{job}\n";
my $ir = jobquery($infoq, $jr, 'i');
my $ar = jobquery($allocdq, $jr, 'a');
my $ident = $jr->{name};
-- 
2.20.1

[OSSTEST PATCH v2 34/41] cs-bisection-step: Move an AND

[Ian Jackson]

This obviously-fine change makes the next commit easier to review.

Signed-off-by: Ian Jackson 
---
v2: New patch.
---
 cs-bisection-step | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 5d4e179e..f11726aa 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -266,8 +266,8 @@ $qtxt_common_tables
   CROSS JOIN tmp_build_info AS url
 
WHERE
-@{ $qtxt_common_rev_ok->('rev') }
-AND  url.name LIKE E'tree\\_%'
+@{ $qtxt_common_rev_ok->('rev') } AND
+ url.name LIKE E'tree\\_%'
 AND  url.use = rev.use
 AND  url.job = rev.job
 AND (rev.name = 'built_revision_' || substr(url.name,6) OR
-- 
2.20.1

[OSSTEST PATCH v2 23/41] Executive: Export opendb_tests

[Ian Jackson]

sg-report-host-history is going to want this in a moment

Signed-off-by: Ian Jackson 
---
 Osstest/Executive.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index 2f81e89d..8e4c5b9a 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -49,7 +49,7 @@ BEGIN {
   task_spec_desc findtask findtask_spec @all_lock_tables
   restrictflight_arg restrictflight_cond
   report_run_getinfo report_altcolour
-  report_altchangecolour
+  report_altchangecolour opendb_tests
   report_blessingscond report_find_push_age_info
   tcpconnect_queuedaemon plan_search
   manual_allocation_base_jobinfo
-- 
2.20.1

[OSSTEST PATCH v2 28/41] sg-report-host-history: Rerganisation: Change loops

[Ian Jackson]

Move the per-host code all into the same per-host loop.  One effect is
to transpose the db_retry and host loops for mainquery.

No functional change.

Signed-off-by: Ian Jackson 
---
 sg-report-host-history | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/sg-report-host-history b/sg-report-host-history
index 3f4670e5..2ca0e235 100755
--- a/sg-report-host-history
+++ b/sg-report-host-history
@@ -470,18 +470,10 @@ db_retry($dbh_tests, [], sub {
 computeflightsrange();
 });
 
-foreach (keys %hosts) {
-read_existing_logs($_);
-}
-
-db_retry($dbh_tests, [], sub {
-foreach my $host (sort keys %hosts) {
-   mainquery($host);
-}
-});
-
 foreach my $host (sort keys %hosts) {
+read_existing_logs($host);
 db_retry($dbh_tests, [], sub {
+mainquery($host);
reporthost $host;
 });
 }
-- 
2.20.1

Re: [PATCH v1] tools/xen-cpuid: show enqcmd

[Andrew Cooper]

On 31/07/2020 13:03, Jan Beulich wrote:
> On 30.07.2020 18:34, Olaf Hering wrote:
>> Translate <29> into a feature string.
>>
>> Signed-off-by: Olaf Hering 
> Acked-by: Jan Beulich 
>
> Albeit I'm pretty sure there are more missing than just this lone one.

And in particular, probably missing from libxl_cpuid.c, which I was
meaning to check when I've got a free moment.

~Andrew

[xen-unstable test] 152311: tolerable FAIL - PUSHED

[osstest service owner]

flight 152311 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152311/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152293
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152293
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152293
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152293
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 152293
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 152293
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152293
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152293
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 152293
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 xen  98bed5de1de3352c63cfe29a00f17e8d9ce72689
baseline version:
 xen  b071ec25e85c4aacf3da59e5258cda0b1c4df45d

Last test of basis   152293  2020-07-30 01:51:37 Z1 days
Testing same since   152311  2020-07-31 01:07:41 Z0 days1 attempts


People who touched revisions under test:
  Fam Zheng 
  Roger Pau Monne 
  Roger Pau Monné 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf

Re: [PATCH v1] tools/xen-cpuid: show enqcmd

[Jan Beulich]

On 30.07.2020 18:34, Olaf Hering wrote:
> Translate <29> into a feature string.
> 
> Signed-off-by: Olaf Hering 

Acked-by: Jan Beulich 

Albeit I'm pretty sure there are more missing than just this lone one.

Jan

> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -133,7 +133,7 @@ static const char *const str_7c0[32] =
>  [22] = "rdpid",
>  /* 24 */   [25] = "cldemote",
>  /* 26 */   [27] = "movdiri",
> -[28] = "movdir64b",
> +[28] = "movdir64b",[29] = "enqcmd",
>  [30] = "sgx-lc",
>  };
>  
> 

[OSSTEST PATCH v2 32/41] adhoc-revtuple-generator: Fix an undef warning in a debug print

[Ian Jackson]

$parents might be undef here.

Signed-off-by: Ian Jackson 
---
New in v2.
---
 adhoc-revtuple-generator | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/adhoc-revtuple-generator b/adhoc-revtuple-generator
index c8d6f4ad..ec33305a 100755
--- a/adhoc-revtuple-generator
+++ b/adhoc-revtuple-generator
@@ -463,7 +463,7 @@ sub coalesce {
$out->{$node}{Date}= $explode_date;
my $parents= $graphs[$explode_i]{ $node[$explode_i] }{Parents};
print DEBUG "#$explode_i $explode_isearliest".
-" $explode_date  x".scalar(@$parents)."\n";
+" $explode_date  x".($parents ? scalar(@$parents) : "-")."\n";
 
foreach my $subparent (@$parents) {
$node[$explode_i]= $subparent;
-- 
2.20.1

[OSSTEST PATCH v2 33/41] cs-bisection-step: Generalise qtxt_common_rev_ok

[Ian Jackson]

* Make it into a subref which takes a $table argument.
* Change the two references into function calls using the @{...} syntax
* Move the definition earlier in the file

No change to the generated query.

Signed-off-by: Ian Jackson 
---
v2: New patch.
---
 cs-bisection-step | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 9a0fee39..5d4e179e 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -182,6 +182,14 @@ END
 sub flight_rmap ($$) {
 my ($flight, $need_urls) = @_;
 
+my $qtxt_common_rev_ok = sub {
+   my ($table) = @_;
+   [('rev') }
 AND  url.name LIKE E'tree\\_%'
 AND  url.use = rev.use
 AND  url.job = rev.job
-- 
2.20.1

[OSSTEST PATCH v2 26/41] sg-report-host-history: Rerganisation: Make mainquery per-host

[Ian Jackson]

This moves the loop over hosts into the main program.  We are working
our way to a new code structure.

No functional change.

Signed-off-by: Ian Jackson 
---
 sg-report-host-history | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/sg-report-host-history b/sg-report-host-history
index 15866ab6..34216aa2 100755
--- a/sg-report-host-history
+++ b/sg-report-host-history
@@ -164,7 +164,9 @@ sub jobquery ($$$) {
 
 our %hosts;
 
-sub mainquery () {
+sub mainquery ($) {
+my ($host) = @_;
+
 our $runvarq //= db_prepare(fetchall_arrayref({});
-   print DEBUG "MAINQUERY $host got ".(scalar @{ $hosts{$host} })."\n";
-}
+print DEBUG "MAINQUERY $host...\n";
+$runvarq->execute($host, $minflight);
+
+$hosts{$host} = $runvarq->fetchall_arrayref({});
+print DEBUG "MAINQUERY $host got ".(scalar @{ $hosts{$host} })."\n";
 }
 
 sub reporthost ($) {
@@ -474,7 +475,9 @@ db_retry($dbh_tests, [], sub {
 });
 
 db_retry($dbh_tests, [], sub {
-mainquery();
+foreach my $host (sort keys %hosts) {
+   mainquery($host);
+}
 });
 
 foreach my $host (sort keys %hosts) {
-- 
2.20.1

[OSSTEST PATCH v2 35/41] cs-bisection-step: Break out qtxt_common_ok

[Ian Jackson]

Make this bit of query into a subref which takes a $table argument.

No change to the generated query.

Signed-off-by: Ian Jackson 
---
v2: New patch.
---
 cs-bisection-step | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index f11726aa..ba0c6424 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -190,6 +190,13 @@ sub flight_rmap ($$) {
 END
 };
 
+my $qtxt_common_tree_ok = sub {
+   my ($table) = @_;
+   [('url') }
 AND  url.use = rev.use
 AND  url.job = rev.job
 AND (rev.name = 'built_revision_' || substr(url.name,6) OR
-- 
2.20.1