[linux-5.4 test] 152486: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-qemuu-nested-amd 14 xen-boot/l1 fail REGR. vs. 152331

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   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-amd64-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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail 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-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  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
 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-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  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-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-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop  fail never pass
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-rtds 14 saverestore-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-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 saverestore-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-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass

version targeted for testing:
 linux1b940bbc5c1a3420f403a2b10cb884cffb01
baseline version:
 linux169b93899c7dfb93a2b57da8e3505da9b2afcf5c

Last test of basis   152331  2020-07-31 17:11:00 Z5 days
Testing same since   152486  2020-08-05 08:10:20 Z0 days1 attempts


People who touched revisions under test:
  Abhishek Ambure 
  Alaa Hleihel 
  Alain Michaud 
  Alex Deucher 
  Andrea Righi 
  Andrew Morton 
  Andrii Nakryiko 
  Armas Spann 
  Arnaldo Carvalho de Melo 
  Atish Patra 
  Aya Levin 
  Balaji Pothunoori 
  Ben Hutchings 

[xen-unstable test] 152484: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 16 
guest-start/debianhvm.repeat fail REGR. vs. 152418
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152418

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152418
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152418
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152418
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 152418
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152418
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 152418
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152418
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152418
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 152418
 test-amd64-i386-xl-pvshim12 guest-start  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-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-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-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  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  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-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   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-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  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-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-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-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  c9f9a7258dc07735e2da2b6d0b51a0218c76a51f
baseline version:
 xen  81fd0d3ca4b2cd309403c6e8da662c325dd35750

Last test of basis   152418  2020-08-03 11:11:04 Z2 days
Failing since152461  2020-08-04 07:50:21 Z1 days2 attempts
Testing same since   152484  2020-08-05 05:20:21 Z0 days1 attempts

Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op

[Stefano Stabellini]

On Wed, 5 Aug 2020, Julien Grall wrote:
> On 05/08/2020 00:22, Stefano Stabellini wrote:
> > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko 
> > > 
> > > This patch adds ability to the device emulator to notify otherend
> > > (some entity running in the guest) using a SPI and implements Arm
> > > specific bits for it. Proposed interface allows emulator to set
> > > the logical level of a one of a domain's IRQ lines.
> > > 
> > > Please note, this is a split/cleanup of Julien's PoC:
> > > "Add support for Guest IO forwarding to a device emulator"
> > > 
> > > Signed-off-by: Julien Grall 
> > > Signed-off-by: Oleksandr Tyshchenko 
> > > ---
> > >   tools/libs/devicemodel/core.c   | 18 ++
> > >   tools/libs/devicemodel/include/xendevicemodel.h |  4 
> > >   tools/libs/devicemodel/libxendevicemodel.map|  1 +
> > >   xen/arch/arm/dm.c   | 22
> > > +-
> > >   xen/common/hvm/dm.c |  1 +
> > >   xen/include/public/hvm/dm_op.h  | 15 +++
> > >   6 files changed, 60 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> > > index 4d40639..30bd79f 100644
> > > --- a/tools/libs/devicemodel/core.c
> > > +++ b/tools/libs/devicemodel/core.c
> > > @@ -430,6 +430,24 @@ int xendevicemodel_set_isa_irq_level(
> > >   return xendevicemodel_op(dmod, domid, 1, , sizeof(op));
> > >   }
> > >   +int xendevicemodel_set_irq_level(
> > > +xendevicemodel_handle *dmod, domid_t domid, uint32_t irq,
> > > +unsigned int level)
> > 
> > It is a pity that having xen_dm_op_set_pci_intx_level and
> > xen_dm_op_set_isa_irq_level already we need to add a third one, but from
> > the names alone I don't think we can reuse either of them.
> 
> The problem is not the name...
> 
> > 
> > It is very similar to set_isa_irq_level. We could almost rename
> > xendevicemodel_set_isa_irq_level to xendevicemodel_set_irq_level or,
> > better, just add an alias to it so that xendevicemodel_set_irq_level is
> > implemented by calling xendevicemodel_set_isa_irq_level. Honestly I am
> > not sure if it is worth doing it though. Any other opinions?
> 
> ... the problem is the interrupt field is only 8-bit. So we would only be able
> to cover IRQ 0 - 255.

Argh, that's not going to work :-(  I wasn't sure if it was a good idea
anyway.


> It is not entirely clear how the existing subop could be extended without
> breaking existing callers.
>
> > But I think we should plan for not needing two calls (one to set level
> > to 1, and one to set it to 0):
> > https://marc.info/?l=xen-devel=159535112027405
> 
> I am not sure to understand your suggestion here? Are you suggesting to remove
> the 'level' parameter?

My hope was to make it optional to call the hypercall with level = 0,
not necessarily to remove 'level' from the struct.

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Stefano Stabellini]

On Wed, 5 Aug 2020, Jan Beulich wrote:
> On 04.08.2020 21:11, Stefano Stabellini wrote:
> >> The point of the check isn't to determine whether to wait, but
> >> what to do after having waited. Reads need a retry round through
> >> the emulator (to store the result in the designated place),
> >> while writes don't have such a requirement (and hence guest
> >> execution can continue immediately in the general case).
> > 
> > The x86 code looks like this:
> > 
> > rc = hvm_send_ioreq(s, , 0);
> > if ( rc != X86EMUL_RETRY || currd->is_shutting_down )
> > vio->io_req.state = STATE_IOREQ_NONE;
> > else if ( !hvm_ioreq_needs_completion(>io_req) )
> > rc = X86EMUL_OKAY;
> > 
> > Basically hvm_send_ioreq is expected to return RETRY.
> > Then, if it is a PIO write operation only, it is turned into OKAY right
> > away. Otherwise, rc stays as RETRY.
> > 
> > So, normally, hvmemul_do_io is expected to return RETRY, because the
> > emulator is not done yet. Am I understanding the code correctly?
> 
> "The emulator" unfortunately is ambiguous here: Do you mean qemu
> (or whichever else ioreq server) or the x86 emulator inside Xen?

I meant QEMU. I'll use "QEMU" instead of "emulator" in this thread going
forward for clarity.


> There are various conditions leading to RETRY. As far as
> hvm_send_ioreq() goes, it is expected to return RETRY whenever
> some sort of response is to be expected (the most notable
> exception being the hvm_send_buffered_ioreq() path), or when
> submitting the request isn't possible in the first place.
> 
> > If so, who is handling RETRY on x86? It tried to follow the call chain
> > but ended up in the x86 emulator and got lost :-)
> 
> Not sure I understand the question correctly, but I'll try an
> answer nevertheless: hvm_send_ioreq() arranges for the vCPU to be
> put to sleep (prepare_wait_on_xen_event_channel()). Once the event
> channel got signaled (and vCPU unblocked), hvm_do_resume() ->
> handle_hvm_io_completion() -> hvm_wait_for_io() then check whether
> the wait reason has been satisfied (wait_on_xen_event_channel()),
> and ...
> 
> > At some point later, after the emulator (QEMU) has completed the
> > request, handle_hvm_io_completion gets called which ends up calling
> > handle_mmio() finishing the job on the Xen side too.
> 
> ..., as you say, handle_hvm_io_completion() invokes the retry of
> the original operation (handle_mmio() or handle_pio() in
> particular) if need be.

OK, thanks for the details. My interpretation seems to be correct.

In which case, it looks like xen/arch/arm/io.c:try_fwd_ioserv should
return IO_RETRY. Then, xen/arch/arm/traps.c:do_trap_stage2_abort_guest
also needs to handle try_handle_mmio returning IO_RETRY the first
around, and IO_HANDLED when after QEMU does its job.

What should do_trap_stage2_abort_guest do on IO_RETRY? Simply return
early and let the scheduler do its job? Something like:

enum io_state state = try_handle_mmio(regs, hsr, gpa);

switch ( state )
{
case IO_ABORT:
goto inject_abt;
case IO_HANDLED:
advance_pc(regs, hsr);
return;
case IO_RETRY:
/* finish later */
return;
case IO_UNHANDLED:
/* IO unhandled, try another way to handle it. */
break;
default:
ASSERT_UNREACHABLE();
}

Then, xen/arch/arm/ioreq.c:handle_mmio() gets called by
handle_hvm_io_completion() after QEMU completes the emulation. Today,
handle_mmio just sets the user register with the read value.

But it would be better if it called again the original function
do_trap_stage2_abort_guest to actually retry the original operation.
This time do_trap_stage2_abort_guest calls try_handle_mmio() and gets
IO_HANDLED instead of IO_RETRY, thus, it will advance_pc (the program
counter) completing the handling of this instruction.

The user register with the read value could be set by try_handle_mmio if
try_fwd_ioserv returns IO_HANDLED instead of IO_RETRY.

Is that how the state machine is expected to work?


> What's potentially confusing is that there's a second form of
> retry, invoked by the x86 insn emulator itself when it needs to
> split complex insns (the repeated string insns being the most
> important example). This results in actually exiting back to guest
> context without having advanced rIP, but after having updated
> other register state suitably (to express the progress made so
> far).

Ah! And it seems to be exactly the same label: X86EMUL_RETRY. It would
be a good idea to differentiate between them.

Re: [RFC PATCH V1 10/12] libxl: Add support for virtio-disk configuration

[Stefano Stabellini]

On Thu, 6 Aug 2020, Oleksandr wrote:
> On 05.08.20 02:23, Stefano Stabellini wrote:
> > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko 
> > > 
> > > This patch adds basic support for configuring and assisting virtio-disk
> > > backend (emualator) which is intended to run out of Qemu and could be run
> > > in any domain.
> > > 
> > > Xenstore was chosen as a communication interface for the emulator running
> > > in non-toolstack domain to be able to get configuration either by reading
> > > Xenstore directly or by receiving command line parameters (an updated 'xl
> > > devd'
> > > running in the same domain would read Xenstore beforehand and call backend
> > > executable with the required arguments).
> > > 
> > > An example of domain configuration (two disks are assigned to the guest,
> > > the latter is in readonly mode):
> > > 
> > > vdisk = [ 'backend=DomD, disks=rw:/dev/mmcblk0p3;ro:/dev/mmcblk1p3' ]
> > > 
> > > Where per-disk Xenstore entries are:
> > > - filename and readonly flag (configured via "vdisk" property)
> > > - base and irq (allocated dynamically)
> > > 
> > > Besides handling 'visible' params described in configuration file,
> > > patch also allocates virtio-mmio specific ones for each device and
> > > writes them into Xenstore. virtio-mmio params (irq and base) are
> > > unique per guest domain, they allocated at the domain creation time
> > > and passed through to the emulator. Each VirtIO device has at least
> > > one pair of these params.
> > > 
> > > TODO:
> > > 1. An extra "virtio" property could be removed.
> > > 2. Update documentation.
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko 
> > > ---
> > >   tools/libxl/Makefile |   4 +-
> > >   tools/libxl/libxl_arm.c  |  63 +++
> > >   tools/libxl/libxl_create.c   |   1 +
> > >   tools/libxl/libxl_internal.h |   1 +
> > >   tools/libxl/libxl_types.idl  |  15 +
> > >   tools/libxl/libxl_types_internal.idl |   1 +
> > >   tools/libxl/libxl_virtio_disk.c  | 109
> > > +
> > >   tools/xl/Makefile|   2 +-
> > >   tools/xl/xl.h|   3 +
> > >   tools/xl/xl_cmdtable.c   |  15 +
> > >   tools/xl/xl_parse.c  | 115
> > > +++
> > >   tools/xl/xl_virtio_disk.c|  46 ++
> > >   12 files changed, 360 insertions(+), 15 deletions(-)
> > >   create mode 100644 tools/libxl/libxl_virtio_disk.c
> > >   create mode 100644 tools/xl/xl_virtio_disk.c
> > > 
> > > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> > > index 38cd43a..df94b13 100644
> > > --- a/tools/libxl/Makefile
> > > +++ b/tools/libxl/Makefile
> > > @@ -141,7 +141,9 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o
> > > libxl_dm.o libxl_pci.o \
> > >   libxl_vtpm.o libxl_nic.o libxl_disk.o 
> > > libxl_console.o
> > > \
> > >   libxl_cpupool.o libxl_mem.o libxl_sched.o 
> > > libxl_tmem.o
> > > \
> > >   libxl_9pfs.o libxl_domain.o libxl_vdispl.o \
> > > - libxl_pvcalls.o libxl_vsnd.o libxl_vkb.o
> > > $(LIBXL_OBJS-y)
> > > + libxl_pvcalls.o libxl_vsnd.o libxl_vkb.o \
> > > + libxl_virtio_disk.o $(LIBXL_OBJS-y)
> > > +
> > >   LIBXL_OBJS += libxl_genid.o
> > >   LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
> > >   diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > > index 4f748e3..469a8b0 100644
> > > --- a/tools/libxl/libxl_arm.c
> > > +++ b/tools/libxl/libxl_arm.c
> > > @@ -13,6 +13,12 @@
> > >   #define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
> > >   #define GUEST_VIRTIO_MMIO_SPI   33
> > >   +#ifndef container_of
> > > +#define container_of(ptr, type, member) ({   \
> > > +typeof( ((type *)0)->member ) *__mptr = (ptr);   \
> > > +(type *)( (char *)__mptr - offsetof(type,member) );})
> > > +#endif
> > > +
> > >   static const char *gicv_to_string(libxl_gic_version gic_version)
> > >   {
> > >   switch (gic_version) {
> > > @@ -44,14 +50,32 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > >   vuart_enabled = true;
> > >   }
> > >   -/*
> > > - * XXX: Handle properly virtio
> > > - * A proper solution would be the toolstack to allocate the
> > > interrupts
> > > - * used by each virtio backend and let the backend now which one is
> > > used
> > > - */
> > >   if (libxl_defbool_val(d_config->b_info.arch_arm.virtio)) {
> > > -nr_spis += (GUEST_VIRTIO_MMIO_SPI - 32) + 1;
> > > +uint64_t virtio_base;
> > > +libxl_device_virtio_disk *virtio_disk;
> > > +
> > > +virtio_base = GUEST_VIRTIO_MMIO_BASE;
> > >   virtio_irq = GUEST_VIRTIO_MMIO_SPI;
> > > +
> > > +if (!d_config->num_virtio_disks) {
> > > +LOG(ERROR, 

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Stefano Stabellini]

On Wed, 5 Aug 2020, Julien Grall wrote:
> On 04/08/2020 20:11, Stefano Stabellini wrote:
> > On Tue, 4 Aug 2020, Julien Grall wrote:
> > > On 04/08/2020 12:10, Oleksandr wrote:
> > > > On 04.08.20 10:45, Paul Durrant wrote:
> > > > > > +static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
> > > > > > +{
> > > > > > +    return ioreq->state == STATE_IOREQ_READY &&
> > > > > > +   !ioreq->data_is_ptr &&
> > > > > > +   (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir !=
> > > > > > IOREQ_WRITE);
> > > > > > +}
> > > > > I don't think having this in common code is correct. The short-cut of
> > > > > not
> > > > > completing PIO reads seems somewhat x86 specific.
> > > 
> > > Hmmm, looking at the code, I think it doesn't wait for PIO writes to
> > > complete
> > > (not read). Did I miss anything?
> > > 
> > > > Does ARM even
> > > > > have the concept of PIO?
> > > > 
> > > > I am not 100% sure here, but it seems that doesn't have.
> > > 
> > > Technically, the PIOs exist on Arm, however they are accessed the same way
> > > as
> > > MMIO and will have a dedicated area defined by the HW.
> > > 
> > > AFAICT, on Arm64, they are only used for PCI IO Bar.
> > > 
> > > Now the question is whether we want to expose them to the Device Emulator
> > > as
> > > PIO or MMIO access. From a generic PoV, a DM shouldn't have to care about
> > > the
> > > architecture used. It should just be able to request a given IOport
> > > region.
> > > 
> > > So it may make sense to differentiate them in the common ioreq code as
> > > well.
> > > 
> > > I had a quick look at QEMU and wasn't able to tell if PIOs and MMIOs
> > > address
> > > space are different on Arm as well. Paul, Stefano, do you know what they
> > > are
> > > doing?
> > 
> > On the QEMU side, it looks like PIO (address_space_io) is used in
> > connection with the emulation of the "in" or "out" instructions, see
> > ioport.c:cpu_inb for instance. Some parts of PCI on QEMU emulate PIO
> > space regardless of the architecture, such as
> > hw/pci/pci_bridge.c:pci_bridge_initfn.
> > 
> > However, because there is no "in" and "out" on ARM, I don't think
> > address_space_io can be accessed. Specifically, there is no equivalent
> > for target/i386/misc_helper.c:helper_inb on ARM.
> 
> So how PCI I/O BAR are accessed? Surely, they could be used on Arm, right?

PIO is also memory mapped on ARM and it seems to have its own MMIO
address window.

Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op

[Oleksandr]

On 05.08.20 19:15, Jan Beulich wrote:

Hi, Jan


On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:

--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -417,6 +417,20 @@ struct xen_dm_op_pin_memory_cacheattr {
  uint32_t pad;
  };
  
+/*

+ * XEN_DMOP_set_irq_level: Set the logical level of a one of a domain's
+ * IRQ lines.
+ * XXX Handle PPIs.
+ */
+#define XEN_DMOP_set_irq_level 19
+
+struct xen_dm_op_set_irq_level {
+uint32_t irq;
+/* IN - Level: 0 -> deasserted, 1 -> asserted */
+uint8_t  level;
+};

If this is the way to go (I've seen other discussion going on),
please make sure you add explicit padding fields and ...


ok





+
+
  struct xen_dm_op {

... you don't add double blank lines.


ok


--
Regards,

Oleksandr Tyshchenko

Re: [RFC PATCH V1 10/12] libxl: Add support for virtio-disk configuration

[Oleksandr]

On 05.08.20 02:23, Stefano Stabellini wrote:

Hi Stefano


On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch adds basic support for configuring and assisting virtio-disk
backend (emualator) which is intended to run out of Qemu and could be run
in any domain.

Xenstore was chosen as a communication interface for the emulator running
in non-toolstack domain to be able to get configuration either by reading
Xenstore directly or by receiving command line parameters (an updated 'xl devd'
running in the same domain would read Xenstore beforehand and call backend
executable with the required arguments).

An example of domain configuration (two disks are assigned to the guest,
the latter is in readonly mode):

vdisk = [ 'backend=DomD, disks=rw:/dev/mmcblk0p3;ro:/dev/mmcblk1p3' ]

Where per-disk Xenstore entries are:
- filename and readonly flag (configured via "vdisk" property)
- base and irq (allocated dynamically)

Besides handling 'visible' params described in configuration file,
patch also allocates virtio-mmio specific ones for each device and
writes them into Xenstore. virtio-mmio params (irq and base) are
unique per guest domain, they allocated at the domain creation time
and passed through to the emulator. Each VirtIO device has at least
one pair of these params.

TODO:
1. An extra "virtio" property could be removed.
2. Update documentation.

Signed-off-by: Oleksandr Tyshchenko 
---
  tools/libxl/Makefile |   4 +-
  tools/libxl/libxl_arm.c  |  63 +++
  tools/libxl/libxl_create.c   |   1 +
  tools/libxl/libxl_internal.h |   1 +
  tools/libxl/libxl_types.idl  |  15 +
  tools/libxl/libxl_types_internal.idl |   1 +
  tools/libxl/libxl_virtio_disk.c  | 109 +
  tools/xl/Makefile|   2 +-
  tools/xl/xl.h|   3 +
  tools/xl/xl_cmdtable.c   |  15 +
  tools/xl/xl_parse.c  | 115 +++
  tools/xl/xl_virtio_disk.c|  46 ++
  12 files changed, 360 insertions(+), 15 deletions(-)
  create mode 100644 tools/libxl/libxl_virtio_disk.c
  create mode 100644 tools/xl/xl_virtio_disk.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 38cd43a..df94b13 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -141,7 +141,9 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_vtpm.o libxl_nic.o libxl_disk.o libxl_console.o \
libxl_cpupool.o libxl_mem.o libxl_sched.o libxl_tmem.o \
libxl_9pfs.o libxl_domain.o libxl_vdispl.o \
-   libxl_pvcalls.o libxl_vsnd.o libxl_vkb.o $(LIBXL_OBJS-y)
+   libxl_pvcalls.o libxl_vsnd.o libxl_vkb.o \
+   libxl_virtio_disk.o $(LIBXL_OBJS-y)
+
  LIBXL_OBJS += libxl_genid.o
  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
  
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c

index 4f748e3..469a8b0 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -13,6 +13,12 @@
  #define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
  #define GUEST_VIRTIO_MMIO_SPI   33
  
+#ifndef container_of

+#define container_of(ptr, type, member) ({ \
+typeof( ((type *)0)->member ) *__mptr = (ptr);  \
+(type *)( (char *)__mptr - offsetof(type,member) );})
+#endif
+
  static const char *gicv_to_string(libxl_gic_version gic_version)
  {
  switch (gic_version) {
@@ -44,14 +50,32 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
  vuart_enabled = true;
  }
  
-/*

- * XXX: Handle properly virtio
- * A proper solution would be the toolstack to allocate the interrupts
- * used by each virtio backend and let the backend now which one is used
- */
  if (libxl_defbool_val(d_config->b_info.arch_arm.virtio)) {
-nr_spis += (GUEST_VIRTIO_MMIO_SPI - 32) + 1;
+uint64_t virtio_base;
+libxl_device_virtio_disk *virtio_disk;
+
+virtio_base = GUEST_VIRTIO_MMIO_BASE;
  virtio_irq = GUEST_VIRTIO_MMIO_SPI;
+
+if (!d_config->num_virtio_disks) {
+LOG(ERROR, "Virtio is enabled, but no Virtio devices present\n");
+return ERROR_FAIL;
+}
+virtio_disk = _config->virtio_disks[0];
+
+for (i = 0; i < virtio_disk->num_disks; i++) {
+virtio_disk->disks[i].base = virtio_base;
+virtio_disk->disks[i].irq = virtio_irq;
+
+LOG(DEBUG, "Allocate Virtio MMIO params: IRQ %u BASE 0x%"PRIx64,
+virtio_irq, virtio_base);
+
+virtio_irq ++;
+virtio_base += GUEST_VIRTIO_MMIO_SIZE;
+}
+virtio_irq --;
+
+nr_spis += (virtio_irq - 32) + 1;

It looks like it is an interrupt per device, which could lead to 

Re: [RFC PATCH V1 09/12] libxl: Handle virtio-mmio irq in more correct way

[Oleksandr]

On 05.08.20 02:22, Stefano Stabellini wrote:

Hi Stefano


On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch makes possible to use device passthrough again.

Signed-off-by: Oleksandr Tyshchenko 
---
  tools/libxl/libxl_arm.c | 33 +++--
  1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 620b499..4f748e3 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -9,6 +9,10 @@
  #include 
  #include 
  
+#define GUEST_VIRTIO_MMIO_BASE  xen_mk_ullong(0x0200)

+#define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
+#define GUEST_VIRTIO_MMIO_SPI   33

They should be in xen/include/public/arch-arm.h


ok




Is one interrupt enough if there are multiple virtio devices? Is it one
interrupt for all virtio devices, or one for each device?


  One interrupt for each virtio device. I experimented with current 
series and assigned 4 disk partitions to the guest. This resulted in 4 
separate device-tree nodes, and each node had individual SPI and MMIO range.





Of course this patch should be folded in the patch to add virtio support
to libxl.


ok


--
Regards,

Oleksandr Tyshchenko

Re: [RFC PATCH V1 11/12] libxl: Insert "dma-coherent" property into virtio-mmio device node

[Oleksandr]

On 05.08.20 02:23, Stefano Stabellini wrote:

Hi Stefano


On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Without "dma-coherent" property present in virtio-mmio device node,
guest assumes it is non-coherent and making non-cacheable accesses
to the vring when the DMA API is used for vring operations.
But virtio-mmio device which runs at the host size is making cacheable
accesses to vring. This all may result in a loss of coherency between
the guest and host.

With this patch we can avoid modifying guest at all, otherwise we
need to force VirtIO framework to not use DMA API for vring operations.

Signed-off-by: Oleksandr Tyshchenko 

This should also be folded in the first patch for libxl


Agree, will do



--
Regards,

Oleksandr Tyshchenko

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

[osstest service owner]

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

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  b2bc1e714462c6cc222e3bbc38d87b039b4fa405
baseline version:
 xen  e58a71274c65e7547fc2e917f051c5c04e2820e2

Last test of basis   152487  2020-08-05 09:00:51 Z0 days
Testing same since   152494  2020-08-05 17:01:20 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

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
   e58a71274c..b2bc1e7144  b2bc1e714462c6cc222e3bbc38d87b039b4fa405 -> smoke

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr]

On 05.08.20 19:13, Jan Beulich wrote:

Hi, Jan


On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:

--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -713,14 +713,14 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct 
domain *d, unsigned int
  }
  }
  
+#endif /* CONFIG_X86 */

+
  static XSM_INLINE int xsm_dm_op(XSM_DEFAULT_ARG struct domain *d)
  {
  XSM_ASSERT_ACTION(XSM_DM_PRIV);
  return xsm_default_action(action, current->domain, d);
  }
  
-#endif /* CONFIG_X86 */

-
  #ifdef CONFIG_ARGO
  static XSM_INLINE int xsm_argo_enable(const struct domain *d)
  {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index a80bcf3..2a9b39d 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -177,8 +177,8 @@ struct xsm_operations {
  int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, 
uint8_t allow);
  int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t 
allow);
  int (*pmu_op) (struct domain *d, unsigned int op);
-int (*dm_op) (struct domain *d);
  #endif
+int (*dm_op) (struct domain *d);
  int (*xen_version) (uint32_t cmd);
  int (*domain_resource_map) (struct domain *d);
  #ifdef CONFIG_ARGO
@@ -688,13 +688,13 @@ static inline int xsm_pmu_op (xsm_default_t def, struct 
domain *d, unsigned int
  return xsm_ops->pmu_op(d, op);
  }
  
+#endif /* CONFIG_X86 */

+
  static inline int xsm_dm_op(xsm_default_t def, struct domain *d)
  {
  return xsm_ops->dm_op(d);
  }
  
-#endif /* CONFIG_X86 */

-
  static inline int xsm_xen_version (xsm_default_t def, uint32_t op)
  {
  return xsm_ops->xen_version(op);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index d4cce68..e3afd06 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -148,8 +148,8 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
  set_to_dummy_if_null(ops, ioport_permission);
  set_to_dummy_if_null(ops, ioport_mapping);
  set_to_dummy_if_null(ops, pmu_op);
-set_to_dummy_if_null(ops, dm_op);
  #endif
+set_to_dummy_if_null(ops, dm_op);
  set_to_dummy_if_null(ops, xen_version);
  set_to_dummy_if_null(ops, domain_resource_map);
  #ifdef CONFIG_ARGO
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index a314bf8..645192a 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1662,14 +1662,13 @@ static int flask_pmu_op (struct domain *d, unsigned int 
op)
  return -EPERM;
  }
  }
+#endif /* CONFIG_X86 */
  
  static int flask_dm_op(struct domain *d)

  {
  return current_has_perm(d, SECCLASS_HVM, HVM__DM);
  }
  
-#endif /* CONFIG_X86 */

-
  static int flask_xen_version (uint32_t op)
  {
  u32 dsid = domain_sid(current->domain);
@@ -1872,8 +1871,8 @@ static struct xsm_operations flask_ops = {
  .ioport_permission = flask_ioport_permission,
  .ioport_mapping = flask_ioport_mapping,
  .pmu_op = flask_pmu_op,
-.dm_op = flask_dm_op,
  #endif
+.dm_op = flask_dm_op,
  .xen_version = flask_xen_version,
  .domain_resource_map = flask_domain_resource_map,
  #ifdef CONFIG_ARGO

All of this looks to belong into patch 2?



Good point. Will move.

--
Regards,

Oleksandr Tyshchenko

[qemu-mainline test] 152480: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds 12 guest-start  fail REGR. vs. 151065

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 151065
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 151065
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 151065
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 151065
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 151065
 test-amd64-i386-xl-pvshim12 guest-start  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-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-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-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-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-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-amd64-amd64-libvirt-vhd 12 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-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-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-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-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-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 qemuufd3cd581f9dcd11286daacaa5272e721c65aece8
baseline version:
 qemuu9e3903136d9acde2fb2dd9e967ba928050a6cb4a

Last test of basis   151065  2020-06-12 22:27:51 Z   53 days
Failing since151101  2020-06-14 08:32:51 Z   52 days   72 attempts
Testing same since   152480  2020-08-05 02:25:42 Z0 days1 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 Duyck 
  Alexandre Mergnat 
  Alexey Kardashevskiy 
  Alexey Krasikov 
  Alistair Francis 
  Allan Peramaki 
  Andrea Bolognani 
  Andreas Schwab 
  Andrew 
  Andrew Jones 
  Andrew Melnychenko 
  Andrey Shinkevich 
  Ani Sinha 
  Anthony PERARD 
  Antoine Damhet 
  Ard Biesheuvel 
  Artyom Tarasenko 
  Atish Patra 
  Aurelien Jarno 
  Babu Moger 
 

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr]

On 05.08.20 19:41, Stefano Stabellini wrote:
Hi Stefano


On Wed, 5 Aug 2020, Jan Beulich wrote:

On 05.08.2020 01:22, Stefano Stabellini wrote:

On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -385,10 +385,11 @@ static inline int set_foreign_p2m_entry(struct domain *d, 
unsigned long gfn,
  mfn_t mfn)
  {
  /*
- * NOTE: If this is implemented then proper reference counting of
- *   foreign entries will need to be implemented.
+ * XXX: handle properly reference. It looks like the page may not always
+ * belong to d.

Just as a reference, and without taking away anything from the comment,
I think that QEMU is doing its own internal reference counting for these
mappings.

Which of course in no way replaces the need to do proper ref counting
in Xen. (Just FAOD, as I'm not sure why you've said what you've said.)

Given the state of the series, which is a RFC, I only meant to say that
the lack of refcounting shouldn't prevent things from working when using
QEMU. In the sense that if somebody wants to give it a try for an early
demo, they should be able to see it running without crashes.


Yes, for the early demo it works fine, however I don't use Qemu.




Of course, refcounting needs to be implemented.


+


--
Regards,

Oleksandr Tyshchenko

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr]

On 05.08.20 17:12, Julien Grall wrote:

Hi,


Hi Julien




On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch makes possible to forward Guest MMIO accesses
to a device emulator on Arm and enables that support for
Arm64.

Also update XSM code a bit to let DM op be used on Arm.
New arch DM op will be introduced in the follow-up patch.

Please note, at the moment build on Arm32 is broken
(see cmpxchg usage in hvm_send_buffered_ioreq()) if someone
wants to enable CONFIG_IOREQ_SERVER due to the lack of
cmpxchg_64 support on Arm32.

Please note, this is a split/cleanup of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Signed-off-by: Julien Grall 
Signed-off-by: Oleksandr Tyshchenko 
---
  tools/libxc/xc_dom_arm.c    |  25 +++---
  xen/arch/arm/Kconfig    |   1 +
  xen/arch/arm/Makefile   |   2 +
  xen/arch/arm/dm.c   |  34 +
  xen/arch/arm/domain.c   |   9 
  xen/arch/arm/hvm.c  |  46 +-
  xen/arch/arm/io.c   |  67 +-
  xen/arch/arm/ioreq.c    |  86 
+

  xen/arch/arm/traps.c    |  17 +++
  xen/common/memory.c |   5 +-
  xen/include/asm-arm/domain.h    |  80 +++
  xen/include/asm-arm/hvm/ioreq.h | 103 


  xen/include/asm-arm/mmio.h  |   1 +
  xen/include/asm-arm/p2m.h   |   7 +--
  xen/include/xsm/dummy.h |   4 +-
  xen/include/xsm/xsm.h   |   6 +--
  xen/xsm/dummy.c |   2 +-
  xen/xsm/flask/hooks.c   |   5 +-
  18 files changed, 476 insertions(+), 24 deletions(-)
  create mode 100644 xen/arch/arm/dm.c
  create mode 100644 xen/arch/arm/ioreq.c
  create mode 100644 xen/include/asm-arm/hvm/ioreq.h


It feels to me the patch is doing quite a few things that are 
indirectly related. Can this be split to make the review easier?


I would like at least the following split from the series:
   - The tools changes
   - The P2M changes
   - The HVMOP plumbing (if we still require them)

Sure, will split.
However, I don't quite understand where we should leave HVMOP plumbing.
If I understand correctly the suggestion was to switch to acquire 
interface instead (which requires a Linux version to be v4.17 at least)?
I suspect, this is all about "xen/privcmd: add 
IOCTL_PRIVCMD_MMAP_RESOURCE" support for Linux?
Sorry, if asking a lot of questions, my developing environment is based 
on Vendor's BSP which uses v4.14 currently.





[...]


diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
new file mode 100644
index 000..2437099
--- /dev/null
+++ b/xen/arch/arm/dm.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2019 Arm ltd.
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License 
along with

+ * this program; If not, see .
+ */
+
+#include 
+#include 


The list of includes sounds strange. Can we make sure to include only 
necessary headers and add the others when they are required?


Sure, I moved arch_dm_op internals to the next patch in this series, but 
forgot to move corresponding headers as well.







+
+int arch_dm_op(struct xen_dm_op *op, struct domain *d,
+   const struct dmop_args *op_args, bool *const_op)
+{
+    return -EOPNOTSUPP;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */



  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
  {
  long rc = 0;
@@ -111,7 +155,7 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)

  if ( rc )
  goto param_fail;
  -    d->arch.hvm.params[a.index] = a.value;
+    rc = hvmop_set_param(d, );
  }
  else
  {
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index ae7ef96..436f669 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -16,6 +16,7 @@
   * GNU General Public License for more details.
   */
  +#include 
  #include 
  #include 
  #include 
@@ -107,6 +108,62 @@ static const struct mmio_handler 
*find_mmio_handler(struct domain *d,

  return handler;
  }
  +#ifdef CONFIG_IOREQ_SERVER


Can we just implement this function in ioreq.c and provide a stub when 
!CONFIG_IOREQ_SERVER?


Sure






+static enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
+ 

Re: EFI executable corruption when live patching is turned off

[Andrew Cooper]

On 05/08/2020 19:19, Trammell Hudson wrote:
> When building xen from head with almost any combination of options, the 
> resulting xen.efi seems properly formed. When CONFIG_LIVEPATCH is turned off, 
> however, the resulting xen.efi is corrupted in some way and binutils no 
> longer wants to work with it:
>
> ~/build/xen-clean/xen$ git rev-parse HEAD
> 81fd0d3ca4b2cd309403c6e8da662c325dd35750
> ~/build/xen-clean/xen$ diff .config.orig .config
> 71,72c71
> < CONFIG_LIVEPATCH=y
> < CONFIG_FAST_SYMBOL_LOOKUP=y
> ---
>> # CONFIG_LIVEPATCH is not set
> 105a105
>> # CONFIG_COVERAGE is not set
> ~/build/xen-clean/xen$ objcopy xen-orig.efi test.efi
> ~/build/xen-clean/xen$ objcopy xen.efi test.efi
> objcopy: test.efi: Data Directory size (1c) exceeds space left in section (18)
> objcopy: test.efi: error copying private BFD data: file in wrong format
> ~/build/xen-clean/xen$ objcopy --version | head -1
> GNU objcopy (GNU Binutils for Ubuntu) 2.34
>
>
> I spent most of today unsuccessfully trying to figure out what was different 
> between the builds (on multiple build host OS with different binutils), so 
> I'm hoping that perhaps someone else has seen this problem.

CC'ing appropriate maintainers.

The difference caused by CONFIG_LIVEPATCH will probably be the logic to
embed the GNU BuildID, which for xen.efi takes a trip through .bin/.ihex
immediately prior to the final link.

~Andrew

EFI executable corruption when live patching is turned off

[Trammell Hudson]

When building xen from head with almost any combination of options, the 
resulting xen.efi seems properly formed. When CONFIG_LIVEPATCH is turned off, 
however, the resulting xen.efi is corrupted in some way and binutils no longer 
wants to work with it:

~/build/xen-clean/xen$ git rev-parse HEAD
81fd0d3ca4b2cd309403c6e8da662c325dd35750
~/build/xen-clean/xen$ diff .config.orig .config
71,72c71
< CONFIG_LIVEPATCH=y
< CONFIG_FAST_SYMBOL_LOOKUP=y
---
> # CONFIG_LIVEPATCH is not set
105a105
> # CONFIG_COVERAGE is not set
~/build/xen-clean/xen$ objcopy xen-orig.efi test.efi
~/build/xen-clean/xen$ objcopy xen.efi test.efi
objcopy: test.efi: Data Directory size (1c) exceeds space left in section (18)
objcopy: test.efi: error copying private BFD data: file in wrong format
~/build/xen-clean/xen$ objcopy --version | head -1
GNU objcopy (GNU Binutils for Ubuntu) 2.34


I spent most of today unsuccessfully trying to figure out what was different 
between the builds (on multiple build host OS with different binutils), so I'm 
hoping that perhaps someone else has seen this problem.

--
Trammell

[RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support

[Trammell Hudson]

This preliminary patch adds support for bundling the Xen hypervisor, xen.cfg, 
the Linux kernel, initrd and XSM into a single "unified" EFI executable that 
can be signed by sbsigntool for verification by UEFI Secure Boot.  It is 
inspired by systemd-boot's unified kernel technique and borrows the function to 
locate PE sections from systemd's LGPL'ed code.

The configuration, kernel, etc are added after building using objcopy to add 
named sections for each input file.  This allows an administrator to update the 
components independently without requiring rebuilding xen. During EFI boot, Xen 
looks at its own loaded image to locate the PE sections and, if secure boot is 
enabled, only allows use of the unified components.

The resulting EFI executable can be invoked directly from the UEFI Boot 
Manager, removing the need to use a separate loader like grub. Unlike the shim 
based verification, the signature covers the entire Xen+config+kernel+initrd 
unified file. This also ensures that properly configured platforms will measure 
the entire runtime into the TPM for unsealing secrets or remote attestation.

It has been tested on qemu OVMF with Secure Boot enabled, as well as on real 
Thinkpad hardware.  The EFI console is very slow, although it works and is able 
to boot into dom0.

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 5a520bf..b7b08b6 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -102,6 +102,7 @@ union string {

 struct file {
 UINTN size;
+bool need_to_free;
 union {
 EFI_PHYSICAL_ADDRESS addr;
 void *ptr;
@@ -330,13 +331,13 @@ static void __init noreturn blexit(const CHAR16 *str)
 if ( !efi_bs )
 efi_arch_halt();

-if ( cfg.addr )
+if ( cfg.addr && cfg.need_to_free)
 efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.addr && kernel.need_to_free)
 efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.addr && ramdisk.need_to_free)
 efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.addr && xsm.need_to_free)
 efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));

 efi_arch_blexit();
@@ -619,6 +620,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 what = what ?: L"Seek";
 else
 {
+file->need_to_free = true;
 file->addr = min(1UL << (32 + PAGE_SHIFT),
  HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
 ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
@@ -665,6 +667,136 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 return true;
 }

+
+struct DosFileHeader {
+UINT8   Magic[2];
+UINT16  LastSize;
+UINT16  nBlocks;
+UINT16  nReloc;
+UINT16  HdrSize;
+UINT16  MinAlloc;
+UINT16  MaxAlloc;
+UINT16  ss;
+UINT16  sp;
+UINT16  Checksum;
+UINT16  ip;
+UINT16  cs;
+UINT16  RelocPos;
+UINT16  nOverlay;
+UINT16  reserved[4];
+UINT16  OEMId;
+UINT16  OEMInfo;
+UINT16  reserved2[10];
+UINT32  ExeHeader;
+} __attribute__((packed));
+
+#define PE_HEADER_MACHINE_I386  0x014c
+#define PE_HEADER_MACHINE_X64   0x8664
+#define PE_HEADER_MACHINE_ARM64 0xaa64
+
+struct PeFileHeader {
+UINT16  Machine;
+UINT16  NumberOfSections;
+UINT32  TimeDateStamp;
+UINT32  PointerToSymbolTable;
+UINT32  NumberOfSymbols;
+UINT16  SizeOfOptionalHeader;
+UINT16  Characteristics;
+} __attribute__((packed));
+
+struct PeHeader {
+UINT8   Magic[4];
+struct PeFileHeader FileHeader;
+} __attribute__((packed));
+
+struct PeSectionHeader {
+UINT8   Name[8];
+UINT32  VirtualSize;
+UINT32  VirtualAddress;
+UINT32  SizeOfRawData;
+UINT32  PointerToRawData;
+UINT32  PointerToRelocations;
+UINT32  PointerToLinenumbers;
+UINT16  NumberOfRelocations;
+UINT16  NumberOfLinenumbers;
+UINT32  Characteristics;
+} __attribute__((packed));
+
+static void * __init pe_find_section(const void * const image_base,
+const char * section_name, UINTN * size_out)
+{
+const CHAR8 * const base = image_base;
+const struct DosFileHeader * dos = (const void*) base;
+const struct PeHeader * pe;
+const UINTN name_len = strlen(section_name);
+UINTN offset;
+
+if ( base == NULL )
+return NULL;
+
+if ( memcmp(dos->Magic, "MZ", 2) != 0 )
+return NULL;
+
+pe = (const void *) [dos->ExeHeader];
+if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 )
+return NULL;
+
+/* PE32+ Subsystem type */
+if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64
+&&  pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64
+&&  pe->FileHeader.Machine 

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Stefano Stabellini]

On Wed, 5 Aug 2020, Jan Beulich wrote:
> On 05.08.2020 01:22, Stefano Stabellini wrote:
> > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> >> --- a/xen/include/asm-arm/p2m.h
> >> +++ b/xen/include/asm-arm/p2m.h
> >> @@ -385,10 +385,11 @@ static inline int set_foreign_p2m_entry(struct 
> >> domain *d, unsigned long gfn,
> >>  mfn_t mfn)
> >>  {
> >>  /*
> >> - * NOTE: If this is implemented then proper reference counting of
> >> - *   foreign entries will need to be implemented.
> >> + * XXX: handle properly reference. It looks like the page may not 
> >> always
> >> + * belong to d.
> > 
> > Just as a reference, and without taking away anything from the comment,
> > I think that QEMU is doing its own internal reference counting for these
> > mappings.
> 
> Which of course in no way replaces the need to do proper ref counting
> in Xen. (Just FAOD, as I'm not sure why you've said what you've said.)

Given the state of the series, which is a RFC, I only meant to say that
the lack of refcounting shouldn't prevent things from working when using
QEMU. In the sense that if somebody wants to give it a try for an early
demo, they should be able to see it running without crashes.

Of course, refcounting needs to be implemented.

RE: [RFC PATCH V1 07/12] A collection of tweaks to be able to run emulator in driver domain

[Paul Durrant]

> -Original Message-
> From: Jan Beulich 
> Sent: 05 August 2020 17:20
> To: Oleksandr Tyshchenko ; Paul Durrant 
> Cc: xen-devel@lists.xenproject.org; Oleksandr Tyshchenko 
> ; Andrew
> Cooper ; George Dunlap ; 
> Ian Jackson
> ; Julien Grall ; Stefano Stabellini
> ; Wei Liu ; Daniel De Graaf 
> 
> Subject: Re: [RFC PATCH V1 07/12] A collection of tweaks to be able to run 
> emulator in driver domain
> 
> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko 
> >
> > Trying to run emulator in driver domain I ran into various issues
> > mostly policy-related. So this patch tries to resolve all them
> > plobably in a hackish way. I would like to get feedback how
> > to implement them properly as having an emulator in driver domain
> > is a completely valid use-case.
> 
> From going over the comments I can only derive you want to run
> an emulator in a driver domain, which doesn't really make sense
> to me. A driver domain has a different purpose after all. If
> instead you mean it to be run in just some other domain (which
> also isn't the domain controlling the target), then there may
> be more infrastructure changes needed.
> 
> Paul - was/is your standalone ioreq server (demu?) able to run
> in other than the domain controlling a guest?
> 

Not something I've done yet, but it was always part of the idea so that we 
could e.g. pass through a device to a dedicated domain and then run multiple 
demu instances there to virtualize it for many domUs. (I'm thinking here of a 
device that is not SR-IOV and hence would need some bespoke emulation code to 
share it out). That dedicated domain would be termed the 'driver domain' simply 
because it is running the device driver for the h/w that underpins the 
emulation.

  Paul

Re: [PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Andrew Cooper]

On 05/08/2020 15:54, Jan Beulich wrote:
> On 05.08.2020 16:18, Andrew Cooper wrote:
>> A Gemini Lake platform prints:
>>
>>   (XEN) CPU0: TSC: 1920MHz * 279 / 3 = 178560MHz
>>   (XEN) CPU0: 800..1800 MHz
>>
>> during boot.  The units on the first line are Hz, not MHz, so correct that 
>> and
>> add a space for clarity.
>>
>> Also, for the min/max line, use three dots instead of two and add more spaces
>> so that the line can't be mistaken for being a double decimal point typo.
>>
>> Boot now reads:
>>
>>   (XEN) CPU0: TSC: 1920 Hz * 279 / 3 = 178560 Hz
>>   (XEN) CPU0: 800 ... 1800 MHz
>>
>> Extend these changes to the other TSC diagnostics.
> I'm happy to see the unit mistake fixed, but the choice of
> formatting was pretty deliberate when the code was introduced:
> As dense as possible without making things unreadable or
> ambiguous. (Considering "a double decimal point typo" looks
> like a joke to me, really.)

I literally thought it was a typo until I read the code.  So no - I'm
very much not joking.

Decimal points are extremely commonly seen with frequencies, and nothing
else in the log line gives any hint that it is range.

Despite being deliberate, it is overly dense and ambiguous as a consequence.

>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>>  
>>  val *= ebx;
>>  do_div(val, eax);
>> -printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
>> +printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
>> smp_processor_id(), ecx, ebx, eax, val);
> For this one I wonder whether ecx wouldn't better be scaled down to
> kHz, and val down to MHz.

That depends on whether we will lose precision in the process.

In principle we can, given ecx's unit of Hz, so I'd be tempted to leave
it as is.

>
>>  }
>>  else if ( ecx | eax | ebx )
>>  {
>>  printk("CPU%u: TSC:", smp_processor_id());
>>  if ( ecx )
>> -printk(" core: %uMHz", ecx);
>> +printk(" core: %u MHz", ecx);
> This one now clearly wants to say Hz too, or (as above) scaling
> down to kHz.

Oops.  Will fix.

> With at least this last issue addressed
> Reviewed-by: Jan Beulich 

Thanks,

~Andrew

Re: [RFC PATCH V1 08/12] xen/arm: Invalidate qemu mapcache on XENMEM_decrease_reservation

[Jan Beulich]

On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  break;
>  }
>  
> +/* x86 already sets the flag in hvm_memory_op() */
> +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER)
> +if ( op == XENMEM_decrease_reservation )
> +curr_d->arch.hvm.qemu_mapcache_invalidate = true;
> +#endif

Doesn't the comment already indicate a route towards an approach
not requiring to alter common code?

Jan

Re: [RFC PATCH V1 07/12] A collection of tweaks to be able to run emulator in driver domain

[Jan Beulich]

On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko 
> 
> Trying to run emulator in driver domain I ran into various issues
> mostly policy-related. So this patch tries to resolve all them
> plobably in a hackish way. I would like to get feedback how
> to implement them properly as having an emulator in driver domain
> is a completely valid use-case.

>From going over the comments I can only derive you want to run
an emulator in a driver domain, which doesn't really make sense
to me. A driver domain has a different purpose after all. If
instead you mean it to be run in just some other domain (which
also isn't the domain controlling the target), then there may
be more infrastructure changes needed.

Paul - was/is your standalone ioreq server (demu?) able to run
in other than the domain controlling a guest?

Jan

RE: [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail

[Paul Durrant]

> -Original Message-
> From: Jan Beulich 
> Sent: 05 August 2020 17:06
> To: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org; Paul Durrant 
> Subject: Re: [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or 
> iommu_unmap() fail
> 
> On 04.08.2020 15:42, Paul Durrant wrote:
> > From: Paul Durrant 
> >
> > This patch adds a full I/O TLB flush to the error paths of iommu_map() and
> > iommu_unmap().
> >
> > Without this change callers need constructs such as:
> >
> > rc = iommu_map/unmap(...)
> > err = iommu_flush(...)
> > if ( !rc )
> >   rc = err;
> >
> > With this change, it can be simplified to:
> >
> > rc = iommu_map/unmap(...)
> > if ( !rc )
> >   rc = iommu_flush(...)
> >
> > because, if the map or unmap fails the flush will be unnecessary. This saves
> > a stack variable and generally makes the call sites tidier.
> 
> I appreciate the intent of tidier code, but I wonder whether this
> flushing doesn't go a little too far: There's a need to flush in
> general when multiple pages were to be (un)mapped, and there was
> at least partial success. Hence e.g. in the order == 0 case I
> don't see why any flushing would be needed. Granted errors aren't
> commonly expected, but anyway.
> 

Yes, I wasn't really worried about optimizing the error case, but I can avoid 
unnecessary flushing in the order 0 case.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -274,6 +274,10 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> >  break;
> >  }
> >
> > +/* Something went wrong so flush everything and clear flush flags */
> > +if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )
> 
> Both here and in the unmap path, did you get the return value
> of iommu_iotlb_flush_all() the wrong way round (i.e. isn't there
> a missing ! )?
> 

Yes, I think you're right. I'll need to re-work anyway to avoid the flush in 
the order 0 case.

  Paul

> Jan

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Andrew Cooper]

On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 06881d0..f6fc3f8 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -70,6 +70,7 @@ extra-y := symbols-dummy.o
>  
>  obj-$(CONFIG_COVERAGE) += coverage/
>  obj-y += sched/
> +obj-$(CONFIG_IOREQ_SERVER) += hvm/
>  obj-$(CONFIG_UBSAN) += ubsan/
>  
>  obj-$(CONFIG_NEEDS_LIBELF) += libelf/
> diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile
> new file mode 100644
> index 000..326215d
> --- /dev/null
> +++ b/xen/common/hvm/Makefile
> @@ -0,0 +1 @@
> +obj-y += ioreq.o
> diff --git a/xen/common/hvm/ioreq.c b/xen/common/hvm/ioreq.c
> new file mode 100644
> index 000..7e1fa23
> --- /dev/null
> +++ b/xen/common/hvm/ioreq.c
> 

HVM is an internal detail of arch specific code.  It should not escape
into common code.

>From x86's point of view, there is nothing conceptually wrong with
having an IOREQ server for PV guests, although it is very unlikely at
this point that adding support would be a good use of time.

Please make this into a proper top-level common set of functionality.

~Andrew

Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op

[Jan Beulich]

On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -417,6 +417,20 @@ struct xen_dm_op_pin_memory_cacheattr {
>  uint32_t pad;
>  };
>  
> +/*
> + * XEN_DMOP_set_irq_level: Set the logical level of a one of a domain's
> + * IRQ lines.
> + * XXX Handle PPIs.
> + */
> +#define XEN_DMOP_set_irq_level 19
> +
> +struct xen_dm_op_set_irq_level {
> +uint32_t irq;
> +/* IN - Level: 0 -> deasserted, 1 -> asserted */
> +uint8_t  level;
> +};

If this is the way to go (I've seen other discussion going on),
please make sure you add explicit padding fields and ...

> +
> +
>  struct xen_dm_op {

... you don't add double blank lines.

Jan

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Jan Beulich]

On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -713,14 +713,14 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG 
> struct domain *d, unsigned int
>  }
>  }
>  
> +#endif /* CONFIG_X86 */
> +
>  static XSM_INLINE int xsm_dm_op(XSM_DEFAULT_ARG struct domain *d)
>  {
>  XSM_ASSERT_ACTION(XSM_DM_PRIV);
>  return xsm_default_action(action, current->domain, d);
>  }
>  
> -#endif /* CONFIG_X86 */
> -
>  #ifdef CONFIG_ARGO
>  static XSM_INLINE int xsm_argo_enable(const struct domain *d)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index a80bcf3..2a9b39d 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -177,8 +177,8 @@ struct xsm_operations {
>  int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, 
> uint8_t allow);
>  int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t 
> allow);
>  int (*pmu_op) (struct domain *d, unsigned int op);
> -int (*dm_op) (struct domain *d);
>  #endif
> +int (*dm_op) (struct domain *d);
>  int (*xen_version) (uint32_t cmd);
>  int (*domain_resource_map) (struct domain *d);
>  #ifdef CONFIG_ARGO
> @@ -688,13 +688,13 @@ static inline int xsm_pmu_op (xsm_default_t def, struct 
> domain *d, unsigned int
>  return xsm_ops->pmu_op(d, op);
>  }
>  
> +#endif /* CONFIG_X86 */
> +
>  static inline int xsm_dm_op(xsm_default_t def, struct domain *d)
>  {
>  return xsm_ops->dm_op(d);
>  }
>  
> -#endif /* CONFIG_X86 */
> -
>  static inline int xsm_xen_version (xsm_default_t def, uint32_t op)
>  {
>  return xsm_ops->xen_version(op);
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index d4cce68..e3afd06 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -148,8 +148,8 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
>  set_to_dummy_if_null(ops, ioport_permission);
>  set_to_dummy_if_null(ops, ioport_mapping);
>  set_to_dummy_if_null(ops, pmu_op);
> -set_to_dummy_if_null(ops, dm_op);
>  #endif
> +set_to_dummy_if_null(ops, dm_op);
>  set_to_dummy_if_null(ops, xen_version);
>  set_to_dummy_if_null(ops, domain_resource_map);
>  #ifdef CONFIG_ARGO
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index a314bf8..645192a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1662,14 +1662,13 @@ static int flask_pmu_op (struct domain *d, unsigned 
> int op)
>  return -EPERM;
>  }
>  }
> +#endif /* CONFIG_X86 */
>  
>  static int flask_dm_op(struct domain *d)
>  {
>  return current_has_perm(d, SECCLASS_HVM, HVM__DM);
>  }
>  
> -#endif /* CONFIG_X86 */
> -
>  static int flask_xen_version (uint32_t op)
>  {
>  u32 dsid = domain_sid(current->domain);
> @@ -1872,8 +1871,8 @@ static struct xsm_operations flask_ops = {
>  .ioport_permission = flask_ioport_permission,
>  .ioport_mapping = flask_ioport_mapping,
>  .pmu_op = flask_pmu_op,
> -.dm_op = flask_dm_op,
>  #endif
> +.dm_op = flask_dm_op,
>  .xen_version = flask_xen_version,
>  .domain_resource_map = flask_domain_resource_map,
>  #ifdef CONFIG_ARGO

All of this looks to belong into patch 2?

Jan

Re: [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail

[Jan Beulich]

On 04.08.2020 15:42, Paul Durrant wrote:
> From: Paul Durrant 
> 
> This patch adds a full I/O TLB flush to the error paths of iommu_map() and
> iommu_unmap().
> 
> Without this change callers need constructs such as:
> 
> rc = iommu_map/unmap(...)
> err = iommu_flush(...)
> if ( !rc )
>   rc = err;
> 
> With this change, it can be simplified to:
> 
> rc = iommu_map/unmap(...)
> if ( !rc )
>   rc = iommu_flush(...)
> 
> because, if the map or unmap fails the flush will be unnecessary. This saves
> a stack variable and generally makes the call sites tidier.

I appreciate the intent of tidier code, but I wonder whether this
flushing doesn't go a little too far: There's a need to flush in
general when multiple pages were to be (un)mapped, and there was
at least partial success. Hence e.g. in the order == 0 case I
don't see why any flushing would be needed. Granted errors aren't
commonly expected, but anyway.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -274,6 +274,10 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
>  break;
>  }
>  
> +/* Something went wrong so flush everything and clear flush flags */
> +if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )

Both here and in the unmap path, did you get the return value
of iommu_iotlb_flush_all() the wrong way round (i.e. isn't there
a missing ! )?

Jan

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr]

On 05.08.20 12:32, Julien Grall wrote:

Hi Julien.


Hi Stefano,

On 05/08/2020 00:22, Stefano Stabellini wrote:

On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch makes possible to forward Guest MMIO accesses
to a device emulator on Arm and enables that support for
Arm64.

Also update XSM code a bit to let DM op be used on Arm.
New arch DM op will be introduced in the follow-up patch.

Please note, at the moment build on Arm32 is broken
(see cmpxchg usage in hvm_send_buffered_ioreq()) if someone


Speaking of buffered_ioreq, if I recall correctly, they were only used
for VGA-related things on x86. It looks like it is still true.

If so, do we need it on ARM? Note that I don't think we can get rid of
it from the interface as it is baked into ioreq, but it might be
possible to have a dummy implementation on ARM. Or maybe not: looking at
xen/common/hvm/ioreq.c it looks like it would be difficult to
disentangle bufioreq stuff from the rest of the code.


We possibly don't need it right now. However, this could possibly be 
used in the future (e.g. virtio notification doorbell).



@@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
   */
  void leave_hypervisor_to_guest(void)
  {
+#ifdef CONFIG_IOREQ_SERVER
+    /*
+ * XXX: Check the return. Shall we call that in
+ * continue_running and context_switch instead?
+ * The benefits would be to avoid calling
+ * handle_hvm_io_completion on every return.
+ */


Yeah, that could be a simple and good optimization


Well, it is not simple as it is sounds :). handle_hvm_io_completion() 
is the function in charge to mark the vCPU as waiting for I/O. So we 
would at least need to split the function.


I wrote this TODO because I wasn't sure about the complexity of 
handle_hvm_io_completion(current). Looking at it again, the main 
complexity is the looping over the IOREQ servers.


I think it would be better to optimize handle_hvm_io_completion() 
rather than trying to hack the context_switch() or continue_running().


[...]


diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5fdb6e8..5823f11 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -385,10 +385,11 @@ static inline int set_foreign_p2m_entry(struct 
domain *d, unsigned long gfn,

  mfn_t mfn)
  {
  /*
- * NOTE: If this is implemented then proper reference counting of
- *   foreign entries will need to be implemented.
+ * XXX: handle properly reference. It looks like the page may 
not always

+ * belong to d.


Just as a reference, and without taking away anything from the comment,
I think that QEMU is doing its own internal reference counting for these
mappings.


I am not sure how this matters here? We can't really trust the DM to 
do the right thing if it is not running in dom0.


But, IIRC, the problem is some of the pages doesn't belong to do a 
domain, so it is not possible to treat them as foreign mapping (e.g. 
you wouldn't be able to grab a reference). This investigation was done 
a couple of years ago, so this may have changed in recent Xen.


As a side note, I am a bit surprised to see most of my original TODOs 
present in the code. What is the plan to solve them?
The plan is to solve most critical TODOs in current series, and rest in 
follow-up series if no objections of course. Any pointers how to solve 
them properly would be much appreciated. Unfortunately, now I have a 
weak understanding how they should be fixed. I see at least 3 major TODO 
here:


1. handle properly reference in set_foreign_p2m_entry()
2. optimize handle_hvm_io_completion()
3. hande properly IO_RETRY in try_fwd_ioserv()


--
Regards,

Oleksandr Tyshchenko

Re: [PATCH v4 02/14] x86/iommu: add common page-table allocator

[Jan Beulich]

On 04.08.2020 15:41, Paul Durrant wrote:
> From: Paul Durrant 
> 
> Instead of having separate page table allocation functions in VT-d and AMD
> IOMMU code, we could use a common allocation function in the general x86 code.
> 
> This patch adds a new allocation function, iommu_alloc_pgtable(), for this
> purpose. The function adds the page table pages to a list. The pages in this
> list are then freed by iommu_free_pgtables(), which is called by
> domain_relinquish_resources() after PCI devices have been de-assigned.
> 
> Signed-off-by: Paul Durrant 

Reviewed-by: Jan Beulich 

[qemu-mainline bisection] complete test-amd64-i386-xl-qemuu-ws16-amd64

[osstest service owner]

branch xen-unstable
xenbranch xen-unstable
job test-amd64-i386-xl-qemuu-ws16-amd64
testid windows-install

Tree: linux git://xenbits.xen.org/linux-pvops.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://git.qemu.org/qemu.git
Tree: seabios git://xenbits.xen.org/osstest/seabios.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

  Bug is in tree:  qemuu git://git.qemu.org/qemu.git
  Bug introduced:  7d3660e79830a069f1848bb4fa1cdf8f666424fb
  Bug not present: 9e3903136d9acde2fb2dd9e967ba928050a6cb4a
  Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/152490/


  (Revision log too long, omitted.)


For bisection revision-tuple graph see:
   
http://logs.test-lab.xenproject.org/osstest/results/bisect/qemu-mainline/test-amd64-i386-xl-qemuu-ws16-amd64.windows-install.html
Revision IDs in each graph node refer, respectively, to the Trees above.


Running cs-bisection-step 
--graph-out=/home/logs/results/bisect/qemu-mainline/test-amd64-i386-xl-qemuu-ws16-amd64.windows-install
 --summary-out=tmp/152490.bisection-summary --basis-template=151065 
--blessings=real,real-bisect qemu-mainline test-amd64-i386-xl-qemuu-ws16-amd64 
windows-install
Searching for failure / basis pass:
 152456 fail [host=fiano1] / 151065 ok.
Failure / basis pass flights: 152456 / 151065
(tree with no url: minios)
Tree: linux git://xenbits.xen.org/linux-pvops.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://git.qemu.org/qemu.git
Tree: seabios git://xenbits.xen.org/osstest/seabios.git
Tree: xen git://xenbits.xen.org/xen.git
Latest c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
e557442e3f7ec7bee2d886978bbd259c6d68c75a 
3c659044118e34603161457db9934a34f816d78b 
5c1c3e4f02e458cf280c677c817ae4fd1ed9bf10 
d9c812dda519a1a73e8370e1b81ddf46eb22ed16 
81fd0d3ca4b2cd309403c6e8da662c325dd35750
Basis pass c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
dafce295e6f447ed8905db4e29241e2c6c2a4389 
3c659044118e34603161457db9934a34f816d78b 
9e3903136d9acde2fb2dd9e967ba928050a6cb4a 
2e3de6253422112ae43e608661ba94ea6b345694 
058023b343d4e366864831db46e9b438e9e3a178
Generating revisions with ./adhoc-revtuple-generator  
git://xenbits.xen.org/linux-pvops.git#c3038e718a19fc596f7b1baba0f83d5146dc7784-c3038e718a19fc596f7b1baba0f83d5146dc7784
 
git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860
 
git://xenbits.xen.org/osstest/ovmf.git#dafce295e6f447ed8905db4e29241e2c6c2a4389-e557442e3f7ec7bee2d886978bbd259c6d68c75a
 git://xenbits.xen.org/qemu-xen-traditional.git#3c659044118e34603161457db99\
 34a34f816d78b-3c659044118e34603161457db9934a34f816d78b 
git://git.qemu.org/qemu.git#9e3903136d9acde2fb2dd9e967ba928050a6cb4a-5c1c3e4f02e458cf280c677c817ae4fd1ed9bf10
 
git://xenbits.xen.org/osstest/seabios.git#2e3de6253422112ae43e608661ba94ea6b345694-d9c812dda519a1a73e8370e1b81ddf46eb22ed16
 
git://xenbits.xen.org/xen.git#058023b343d4e366864831db46e9b438e9e3a178-81fd0d3ca4b2cd309403c6e8da662c325dd35750
>From git://cache:9419/git://xenbits.xen.org/xen
   c9f9a7258d..e58a71274c  smoke  -> origin/smoke
Use of uninitialized value $parents in array dereference at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value in concatenation (.) or string at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value $parents in array dereference at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value in concatenation (.) or string at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value $parents in array dereference at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value in concatenation (.) or string at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value $parents in array dereference at 
./adhoc-revtuple-generator line 465.
Use of uninitialized value in concatenation (.) or string at 
./adhoc-revtuple-generator line 465.
Loaded 55467 nodes in revision graph
Searching for test results:
 151101 fail irrelevant
 151065 pass c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
dafce295e6f447ed8905db4e29241e2c6c2a4389 
3c659044118e34603161457db9934a34f816d78b 
9e3903136d9acde2fb2dd9e967ba928050a6cb4a 
2e3de6253422112ae43e608661ba94ea6b345694 
058023b343d4e366864831db46e9b438e9e3a178
 151149 [host=fiano0]
 151221 fail irrelevant
 151175 [host=fiano0]
 151241 fail irrelevant
 151286 fail irrelevant
 151269 fail irrelevant
 151328 fail irrelevant
 151304 fail c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 

Re: [PATCH] x86/ioapic: Improve code generation for __io_apic_{read,write}()

[Jan Beulich]

On 05.08.2020 15:54, Andrew Cooper wrote:
> The write into REGSEL prevents the optimiser from reusing the address
> calculation, forcing it to be calcualted twice.
> 
> The calculation itself is quite expensive.  Pull it out into a local varaible.
> 
> Bloat-o-meter reports:
>   add/remove: 0/0 grow/shrink: 0/26 up/down: 0/-1527 (-1527)
> 
> Also correct the register type, which is uint32_t, not int.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

Re: [PATCH] x86/ioapic: Fix style in io_apic.h

[Jan Beulich]

On 05.08.2020 14:51, Andrew Cooper wrote:
> This file is a mix of Xen and Linux styles.  Switch it fully to Xen style.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 
with a suggestion and, I'm afraid, a few more adjustments:

> --- a/xen/include/asm-x86/io_apic.h
> +++ b/xen/include/asm-x86/io_apic.h
> @@ -13,9 +13,9 @@
>   * Copyright (C) 1997, 1998, 1999, 2000 Ingo Molnar
>   */
>  
> -#define IO_APIC_BASE(idx) \
> - ((volatile int *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + idx) \
> - + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
> +#define IO_APIC_BASE(idx)   \
> +((volatile int *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + idx)   \
> +  + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))

As you touch this anyway, would you mind parenthesizing idx when
used as a, operand of + ?

> @@ -135,28 +135,28 @@ unsigned int io_apic_gsi_base(unsigned int apic);
>  
>  static inline unsigned int __io_apic_read(unsigned int apic, unsigned int 
> reg)
>  {
> - *IO_APIC_BASE(apic) = reg;
> - return *(IO_APIC_BASE(apic)+4);
> +*IO_APIC_BASE(apic) = reg;
> +return *(IO_APIC_BASE(apic)+4);

Here and below + wants to be framed by spaces.

Jan

Re: [PATCH] x86/ioapic: Fix fixmap error path logic in ioapic_init_mappings()

[Jan Beulich]

On 05.08.2020 14:51, Andrew Cooper wrote:
> In the case that bad_ioapic_register() fails, the current position of idx++
> means that clear_fixmap(idx) will be called with the wrong index, and not
> clean up the mapping just created.
> 
> Increment idx as part of the loop, rather than midway through the loop body.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 

Re: [PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Jan Beulich]

On 05.08.2020 16:18, Andrew Cooper wrote:
> A Gemini Lake platform prints:
> 
>   (XEN) CPU0: TSC: 1920MHz * 279 / 3 = 178560MHz
>   (XEN) CPU0: 800..1800 MHz
> 
> during boot.  The units on the first line are Hz, not MHz, so correct that and
> add a space for clarity.
> 
> Also, for the min/max line, use three dots instead of two and add more spaces
> so that the line can't be mistaken for being a double decimal point typo.
> 
> Boot now reads:
> 
>   (XEN) CPU0: TSC: 1920 Hz * 279 / 3 = 178560 Hz
>   (XEN) CPU0: 800 ... 1800 MHz
> 
> Extend these changes to the other TSC diagnostics.

I'm happy to see the unit mistake fixed, but the choice of
formatting was pretty deliberate when the code was introduced:
As dense as possible without making things unreadable or
ambiguous. (Considering "a double decimal point typo" looks
like a joke to me, really.)

> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>  
>  val *= ebx;
>  do_div(val, eax);
> -printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
> +printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
> smp_processor_id(), ecx, ebx, eax, val);

For this one I wonder whether ecx wouldn't better be scaled down to
kHz, and val down to MHz.

>  }
>  else if ( ecx | eax | ebx )
>  {
>  printk("CPU%u: TSC:", smp_processor_id());
>  if ( ecx )
> -printk(" core: %uMHz", ecx);
> +printk(" core: %u MHz", ecx);

This one now clearly wants to say Hz too, or (as above) scaling
down to kHz. With at least this last issue addressed
Reviewed-by: Jan Beulich 
albeit I'd much prefer if the formatting adjustments were dropped.

Jan

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Jan Beulich]

On 05.08.2020 16:12, Julien Grall wrote:
> On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
>> --- /dev/null
>> +++ b/xen/include/asm-arm/hvm/ioreq.h
>> @@ -0,0 +1,103 @@
>> +/*
>> + * hvm.h: Hardware virtual machine assist interface definitions.
>> + *
>> + * Copyright (c) 2016 Citrix Systems Inc.
>> + * Copyright (c) 2019 Arm ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along 
>> with
>> + * this program; If not, see .
>> + */
>> +
>> +#ifndef __ASM_ARM_HVM_IOREQ_H__
>> +#define __ASM_ARM_HVM_IOREQ_H__
>> +
>> +#include 
>> +#include 
>> +
>> +#define has_vpci(d) (false)
> 
> It feels to me this wants to be defined in vcpi.h.

On x86 it wants to live together with a bunch of other has_v*()
macros.

Jan

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

[kernel test robot]

Hi Oleksandr,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next 
tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.8 next-20200804]
[cannot apply to xen-tip/linux-next drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Oleksandr-Andrushchenko/Fixes-and-improvements-for-Xen-pvdrm/20200731-205350
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git 
exynos-drm-next
compiler: aarch64-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 


cppcheck warnings: (new ones prefixed by >>)

>> drivers/irqchip/irq-gic.c:161:24: warning: Local variable gic_data shadows 
>> outer variable [shadowVar]
struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
  ^
   drivers/irqchip/irq-gic.c:123:29: note: Shadowed declaration
   static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
   ^
   drivers/irqchip/irq-gic.c:161:24: note: Shadow variable
struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
  ^
   drivers/irqchip/irq-gic.c:167:24: warning: Local variable gic_data shadows 
outer variable [shadowVar]
struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
  ^
   drivers/irqchip/irq-gic.c:123:29: note: Shadowed declaration
   static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
   ^
   drivers/irqchip/irq-gic.c:167:24: note: Shadow variable
struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
  ^
>> drivers/irqchip/irq-gic.c:400:28: warning: Local variable gic_irq shadows 
>> outer function [shadowFunction]
unsigned int cascade_irq, gic_irq;
  ^
   drivers/irqchip/irq-gic.c:171:28: note: Shadowed declaration
   static inline unsigned int gic_irq(struct irq_data *d)
  ^
   drivers/irqchip/irq-gic.c:400:28: note: Shadow variable
unsigned int cascade_irq, gic_irq;
  ^
>> drivers/irqchip/irq-gic.c:1507:14: warning: Local variable gic_cpu_base 
>> shadows outer function [shadowFunction]
phys_addr_t gic_cpu_base;
^
   drivers/irqchip/irq-gic.c:165:29: note: Shadowed declaration
   static inline void __iomem *gic_cpu_base(struct irq_data *d)
   ^
   drivers/irqchip/irq-gic.c:1507:14: note: Shadow variable
phys_addr_t gic_cpu_base;
^
>> drivers/irqchip/irq-gic-v3.c:874:71: warning: Boolean result is used in 
>> bitwise operation. Clarify expression with parentheses. [clarifyCondition]
gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
 ^
>> drivers/irqchip/irq-gic-v3.c:1808:6: warning: Local variable 
>> nr_redist_regions shadows outer variable [shadowVar]
u32 nr_redist_regions;
^
   drivers/irqchip/irq-gic-v3.c:1880:6: note: Shadowed declaration
u32 nr_redist_regions;
^
   drivers/irqchip/irq-gic-v3.c:1808:6: note: Shadow variable
u32 nr_redist_regions;
^
>> drivers/irqchip/irq-gic-v3.c:2042:6: warning: Local variable maint_irq_mode 
>> shadows outer variable [shadowVar]
int maint_irq_mode;
^
   drivers/irqchip/irq-gic-v3.c:1884:6: note: Shadowed declaration
int maint_irq_mode;
^
   drivers/irqchip/irq-gic-v3.c:2042:6: note: Shadow variable
int maint_irq_mode;
^
>> drivers/gpu/drm/xen/xen_drm_front_cfg.c:76:6: warning: Variable 'ret' is 
>> reassigned a value before the old one has been used. [redundantAssignment]
ret = xen_drm_front_get_edid(front_info, index, pages,
^
   drivers/gpu/drm/xen/xen_drm_front_cfg.c:61:0: note: Variable 'ret' is 
reassigned a value before the old one has been used.
int i, npages, ret = -ENOMEM;
   ^
   drivers/gpu/drm/xen/xen_drm_front_cfg.c:76:6: note: Variable 'ret' is 
reassigned a value before the old one has been used.
ret = xen_drm_front_get_edid(front_info, index, pages,
^

vim +/ret +76 drivers/gpu/drm/xen/xen_drm_front_cfg.c

54  
55  static void cfg_connector_edid(struct xen_drm_front_info *front_info,
56 struct xen_drm_front_cfg_connector 
*connector,
57 int index)
58  {
59  struct page **pages;
60  u32 edid_sz;
61  int i, npages, ret = -ENOMEM;
62  
63  connector->edid = vmalloc(XENDISPL_EDID_MAX_SIZE);
64  if (!connector->edid)
65  

[PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Andrew Cooper]

A Gemini Lake platform prints:

  (XEN) CPU0: TSC: 1920MHz * 279 / 3 = 178560MHz
  (XEN) CPU0: 800..1800 MHz

during boot.  The units on the first line are Hz, not MHz, so correct that and
add a space for clarity.

Also, for the min/max line, use three dots instead of two and add more spaces
so that the line can't be mistaken for being a double decimal point typo.

Boot now reads:

  (XEN) CPU0: TSC: 1920 Hz * 279 / 3 = 178560 Hz
  (XEN) CPU0: 800 ... 1800 MHz

Extend these changes to the other TSC diagnostics.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/arch/x86/cpu/amd.c   |  4 ++--
 xen/arch/x86/cpu/intel.c | 12 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 0cc6853c42..8bc51bec10 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -624,10 +624,10 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
if (idx && idx < h &&
!rdmsr_safe(0xC0010064 + idx, val) && (val >> 63) &&
!rdmsr_safe(0xC0010064, hi) && (hi >> 63))
-   printk("CPU%u: %lu (%lu..%lu) MHz\n",
+   printk("CPU%u: %lu (%lu ... %lu) MHz\n",
   smp_processor_id(), FREQ(val), FREQ(lo), FREQ(hi));
else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
-   printk("CPU%u: %lu..%lu MHz\n",
+   printk("CPU%u: %lu ... %lu MHz\n",
   smp_processor_id(), FREQ(lo), FREQ(hi));
else
printk("CPU%u: %lu MHz\n", smp_processor_id(), FREQ(lo));
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 69e99bb358..ed70b43942 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
 
 val *= ebx;
 do_div(val, eax);
-printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n",
+printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
smp_processor_id(), ecx, ebx, eax, val);
 }
 else if ( ecx | eax | ebx )
 {
 printk("CPU%u: TSC:", smp_processor_id());
 if ( ecx )
-printk(" core: %uMHz", ecx);
+printk(" core: %u MHz", ecx);
 if ( ebx && eax )
 printk(" ratio: %u / %u", ebx, eax);
 printk("\n");
@@ -417,11 +417,11 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
 {
 printk("CPU%u:", smp_processor_id());
 if ( ecx )
-printk(" bus: %uMHz", ecx);
+printk(" bus: %u MHz", ecx);
 if ( eax )
-printk(" base: %uMHz", eax);
+printk(" base: %u MHz", eax);
 if ( ebx )
-printk(" max: %uMHz", ebx);
+printk(" max: %u MHz", ebx);
 printk("\n");
 }
 }
@@ -446,7 +446,7 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
 
 printk("CPU%u: ", smp_processor_id());
 if ( min_ratio )
-printk("%u..", (factor * min_ratio + 50) / 100);
+printk("%u ... ", (factor * min_ratio + 50) / 100);
 printk("%u MHz\n", (factor * max_ratio + 50) / 100);
 }
 }
-- 
2.11.0

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Julien Grall]

Hi,

On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch makes possible to forward Guest MMIO accesses
to a device emulator on Arm and enables that support for
Arm64.

Also update XSM code a bit to let DM op be used on Arm.
New arch DM op will be introduced in the follow-up patch.

Please note, at the moment build on Arm32 is broken
(see cmpxchg usage in hvm_send_buffered_ioreq()) if someone
wants to enable CONFIG_IOREQ_SERVER due to the lack of
cmpxchg_64 support on Arm32.

Please note, this is a split/cleanup of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Signed-off-by: Julien Grall 
Signed-off-by: Oleksandr Tyshchenko 
---
  tools/libxc/xc_dom_arm.c|  25 +++---
  xen/arch/arm/Kconfig|   1 +
  xen/arch/arm/Makefile   |   2 +
  xen/arch/arm/dm.c   |  34 +
  xen/arch/arm/domain.c   |   9 
  xen/arch/arm/hvm.c  |  46 +-
  xen/arch/arm/io.c   |  67 +-
  xen/arch/arm/ioreq.c|  86 +
  xen/arch/arm/traps.c|  17 +++
  xen/common/memory.c |   5 +-
  xen/include/asm-arm/domain.h|  80 +++
  xen/include/asm-arm/hvm/ioreq.h | 103 
  xen/include/asm-arm/mmio.h  |   1 +
  xen/include/asm-arm/p2m.h   |   7 +--
  xen/include/xsm/dummy.h |   4 +-
  xen/include/xsm/xsm.h   |   6 +--
  xen/xsm/dummy.c |   2 +-
  xen/xsm/flask/hooks.c   |   5 +-
  18 files changed, 476 insertions(+), 24 deletions(-)
  create mode 100644 xen/arch/arm/dm.c
  create mode 100644 xen/arch/arm/ioreq.c
  create mode 100644 xen/include/asm-arm/hvm/ioreq.h


It feels to me the patch is doing quite a few things that are indirectly 
related. Can this be split to make the review easier?


I would like at least the following split from the series:
   - The tools changes
   - The P2M changes
   - The HVMOP plumbing (if we still require them)

[...]


diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
new file mode 100644
index 000..2437099
--- /dev/null
+++ b/xen/arch/arm/dm.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2019 Arm ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see .
+ */
+
+#include 
+#include 


The list of includes sounds strange. Can we make sure to include only 
necessary headers and add the others when they are required?



+
+int arch_dm_op(struct xen_dm_op *op, struct domain *d,
+   const struct dmop_args *op_args, bool *const_op)
+{
+return -EOPNOTSUPP;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */



  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
  {
  long rc = 0;
@@ -111,7 +155,7 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
  if ( rc )
  goto param_fail;
  
-d->arch.hvm.params[a.index] = a.value;

+rc = hvmop_set_param(d, );
  }
  else
  {
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index ae7ef96..436f669 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -16,6 +16,7 @@
   * GNU General Public License for more details.
   */
  
+#include 

  #include 
  #include 
  #include 
@@ -107,6 +108,62 @@ static const struct mmio_handler *find_mmio_handler(struct 
domain *d,
  return handler;
  }
  
+#ifdef CONFIG_IOREQ_SERVER


Can we just implement this function in ioreq.c and provide a stub when 
!CONFIG_IOREQ_SERVER?



+static enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
+struct vcpu *v, mmio_info_t *info)
+{
+struct hvm_vcpu_io *vio = >arch.hvm.hvm_io;
+ioreq_t p = {
+.type = IOREQ_TYPE_COPY,
+.addr = info->gpa,
+.size = 1 << info->dabt.size,
+.count = 0,
+.dir = !info->dabt.write,
+.df = 0, /* XXX: What's for? */
+.data = get_user_reg(regs, info->dabt.reg),
+.state = STATE_IOREQ_READY,
+};
+struct hvm_ioreq_server *s = NULL;
+enum io_state rc;
+
+switch ( vio->io_req.state )
+{
+case STATE_IOREQ_NONE:
+break;
+default:
+printk("d%u wrong state %u\n", v->domain->domain_id,
+  

[PATCH] x86/ioapic: Improve code generation for __io_apic_{read, write}()

[Andrew Cooper]

The write into REGSEL prevents the optimiser from reusing the address
calculation, forcing it to be calcualted twice.

The calculation itself is quite expensive.  Pull it out into a local varaible.

Bloat-o-meter reports:
  add/remove: 0/0 grow/shrink: 0/26 up/down: 0/-1527 (-1527)

Also correct the register type, which is uint32_t, not int.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/include/asm-x86/io_apic.h | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index daf17d4c3d..cb36e4ca1b 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -14,8 +14,8 @@
  */
 
 #define IO_APIC_BASE(idx)   \
-((volatile int *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + idx)   \
-  + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
+((volatile uint32_t *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + idx)  \
+   + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
 
 #define IO_APIC_ID(idx) (mp_ioapics[idx].mpc_apicid)
 
@@ -135,8 +135,10 @@ unsigned int io_apic_gsi_base(unsigned int apic);
 
 static inline unsigned int __io_apic_read(unsigned int apic, unsigned int reg)
 {
-*IO_APIC_BASE(apic) = reg;
-return *(IO_APIC_BASE(apic)+4);
+volatile uint32_t *regs = IO_APIC_BASE(apic);
+
+regs[0] = reg;
+return regs[4];
 }
 
 static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
@@ -148,8 +150,10 @@ static inline unsigned int io_apic_read(unsigned int apic, 
unsigned int reg)
 
 static inline void __io_apic_write(unsigned int apic, unsigned int reg, 
unsigned int value)
 {
-*IO_APIC_BASE(apic) = reg;
-*(IO_APIC_BASE(apic)+4) = value;
+volatile uint32_t *regs = IO_APIC_BASE(apic);
+
+regs[0] = reg;
+regs[4] = value;
 }
 
 static inline void io_apic_write(unsigned int apic, unsigned int reg, unsigned 
int value)
-- 
2.11.0

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Durrant, Paul]

> -Original Message-
> From: Ian Jackson 
> Sent: 05 August 2020 10:31
> To: p...@xen.org
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul ; 
> 'Wei Liu' 
> Subject: RE: [EXTERNAL] [PATCH v2 4/4] tools/hotplug: modify set_mtu() to 
> inform the frontend via
> xenstore
> 
> 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.
> 
> 
> 
> Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to 
> inform the frontend via
> xenstore"):
> > > -Original Message-
> > > From: Ian Jackson 
> ...
> > > Actually.
> > >
> > > This shouldn't be in the frontend at all, should it ?  In general the
> > > backend writes to the backend and the frontend to the frontend.
> > >
> > > So maybe I need to take back my R-b of
> > >   [PATCH v2 3/4] public/io/netif: specify MTU override node
> > >
> > > Sorry for the confusion.  I seem rather undercaffienated today.
> >
> > Too late. The xenstore node has been used by Windows frontends for the best 
> > part of a decade so we
> can't practically change the
> > path. Another way would be to also modify netback to simply echo the value 
> > from backend into
> frontend, but that seems rather
> > pointless.
> 
> Hmm.  How does this interact with driver domains ?  I think a driver
> domain might not have write access to this node.
> 

That's a good point; I think we will also need to actually write it from libxl 
first in that case.

> Is there a value we can store in it that won't break these Windows
> frontends, that libxl in the toolstack domain could write, before the
> hotplug script runs in the driver domain ?
> 
> > Interestingly libxl does define an 'mtu' field for libxl_device_nic, which 
> > it sets to 1492 in
> libxl__device_nic_setdefault() but
> > never writes it into xenstore. There is even a comment:
> >
> > /* nic->mtu = */
> >
> > in libxl__nic_from_xenstore() which implies it should have been there, but 
> > isn't.
> > I still think picking up the MTU from the bridge is the better way though.
> 
> I agree that the default should come from the bridge.  Ideally there
> would be a way to override it in the config.
> 

Well, I guess we address the driver domain issue in this way too... I will add 
a patch to libxl to write the libxl_device_nic mtu value into xenstore, in both 
backend (where it should always have been) and frontend. I think the current 
setting of 1492 can be changed to 1500 safely (since nothing appears to 
currently use that value). The hotplug script should then have sufficient 
access to update, and a subsequent patch can add a mechanism to set the value 
from the config.

  Paul

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

[Boris Ostrovsky]

On 8/4/20 7:42 PM, Anchal Agarwal wrote:
>
> I think this could be done. PM_HIBERNATION_PREPARE could return -ENOTSUPP
> for arm and pvh dom0 when the notifier call chain is invoked for this case
> in hibernate(). This will then be an empty notifier just for checking two
> usecases.
> Also, for pvh dom0, the earlier code didn't register any notifier,
> with this approach you are suggesting setup the notifier for hvm/pvh dom0 and
> arm but fail during notifier call chain during PM_HIBERNATION_PREPARE ?


Right.


(Although the earlier code did register the notifier:
xen_setup_pm_notifier() would return an error for !xen_hvm_domain() and
PVH *is* an HVM domain, so registration would actually happen)


>
> I think still getting rid of suspend mode that was earlier a part of this
> notifier is a good idea as it seems redundant as you pointed out earlier. 


Yes.


-boris

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Julien Grall]

Hi,

On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

As a lot of x86 code can be re-used on Arm later on, this patch
splits IOREQ support into common and arch specific parts.

This support is going to be used on Arm to be able run device
emulator outside of Xen hypervisor.

Please note, this is a split/cleanup of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Signed-off-by: Julien Grall 
Signed-off-by: Oleksandr Tyshchenko 
---
  xen/arch/x86/Kconfig|1 +
  xen/arch/x86/hvm/dm.c   |2 +-
  xen/arch/x86/hvm/emulate.c  |2 +-
  xen/arch/x86/hvm/hvm.c  |2 +-
  xen/arch/x86/hvm/io.c   |2 +-
  xen/arch/x86/hvm/ioreq.c| 1431 +--
  xen/arch/x86/hvm/stdvga.c   |2 +-
  xen/arch/x86/hvm/vmx/realmode.c |1 +
  xen/arch/x86/hvm/vmx/vvmx.c |2 +-
  xen/arch/x86/mm.c   |2 +-
  xen/arch/x86/mm/shadow/common.c |2 +-
  xen/common/Kconfig  |3 +
  xen/common/Makefile |1 +
  xen/common/hvm/Makefile |1 +
  xen/common/hvm/ioreq.c  | 1430 ++
  xen/include/asm-x86/hvm/ioreq.h |   45 +-
  xen/include/asm-x86/hvm/vcpu.h  |7 -
  xen/include/xen/hvm/ioreq.h |   89 +++
  18 files changed, 1575 insertions(+), 1450 deletions(-)


That's quite a lot of code moved in a single patch. How can we check the 
code moved is still correct? Is it a verbatim copy?



  create mode 100644 xen/common/hvm/Makefile
  create mode 100644 xen/common/hvm/ioreq.c
  create mode 100644 xen/include/xen/hvm/ioreq.h


[...]


+static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
+{
+unsigned int prev_state = STATE_IOREQ_NONE;
+
+while ( sv->pending )
+{
+unsigned int state = p->state;
+
+smp_rmb();
+
+recheck:
+if ( unlikely(state == STATE_IOREQ_NONE) )
+{
+/*
+ * The only reason we should see this case is when an
+ * emulator is dying and it races with an I/O being
+ * requested.
+ */
+hvm_io_assist(sv, ~0ul);
+break;
+}
+
+if ( unlikely(state < prev_state) )
+{
+gdprintk(XENLOG_ERR, "Weird HVM ioreq state transition %u -> %u\n",
+ prev_state, state);
+sv->pending = false;
+domain_crash(sv->vcpu->domain);
+return false; /* bail */
+}
+
+switch ( prev_state = state )
+{
+case STATE_IORESP_READY: /* IORESP_READY -> NONE */
+p->state = STATE_IOREQ_NONE;
+hvm_io_assist(sv, p->data);
+break;
+case STATE_IOREQ_READY:  /* IOREQ_{READY,INPROCESS} -> IORESP_READY */
+case STATE_IOREQ_INPROCESS:
+wait_on_xen_event_channel(sv->ioreq_evtchn,
+  ({ state = p->state;
+ smp_rmb();
+ state != prev_state; }));
+goto recheck;


I recall some discussion on security@ about this specific code. An IOREQ 
server can be destroyed at any time. When destroying IOREQ server, the 
all the vCPUs will be paused to avoid race.


On x86, this was considered to be safe because
wait_on_xen_event_channel() will never return if the vCPU is re-scheduled.

However, on Arm, this function will return even after rescheduling. In 
this case, sv and p may point to invalid memory.


IIRC, the suggestion was to harden hvm_wait_for_io(). I guess we could 
fetch the sv and p after wait_on_xen_event_channel.


Any opinions?

Cheers,

--
Julien Grall

[PATCH] x86/ioapic: Fix fixmap error path logic in ioapic_init_mappings()

[Andrew Cooper]

In the case that bad_ioapic_register() fails, the current position of idx++
means that clear_fixmap(idx) will be called with the wrong index, and not
clean up the mapping just created.

Increment idx as part of the loop, rather than midway through the loop body.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/arch/x86/io_apic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 878ee5192d..e66fa99ec7 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2543,7 +2543,7 @@ static void __init ioapic_init_mappings(void)
 
 nr_irqs_gsi = 0;
 
-for ( i = 0; i < nr_ioapics; i++ )
+for ( i = 0; i < nr_ioapics; i++, idx++ )
 {
 union IO_APIC_reg_01 reg_01;
 paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr;
@@ -2560,7 +2560,6 @@ static void __init ioapic_init_mappings(void)
 set_fixmap_nocache(idx, ioapic_phys);
 apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08Lx (%08lx)\n",
 __fix_to_virt(idx), ioapic_phys);
-idx++;
 
 if ( bad_ioapic_register(i) )
 {
-- 
2.11.0

[PATCH] x86/ioapic: Fix style in io_apic.h

[Andrew Cooper]

This file is a mix of Xen and Linux styles.  Switch it fully to Xen style.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
---
 xen/include/asm-x86/io_apic.h | 48 +--
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index e006b2b8dd..daf17d4c3d 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -13,9 +13,9 @@
  * Copyright (C) 1997, 1998, 1999, 2000 Ingo Molnar
  */
 
-#define IO_APIC_BASE(idx) \
-   ((volatile int *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + idx) \
-   + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
+#define IO_APIC_BASE(idx)   \
+((volatile int *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + idx)   \
+  + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
 
 #define IO_APIC_ID(idx) (mp_ioapics[idx].mpc_apicid)
 
@@ -78,14 +78,14 @@ extern int nr_ioapics;
 extern int nr_ioapic_entries[MAX_IO_APICS];
 
 enum ioapic_irq_destination_types {
-   dest_Fixed = 0,
-   dest_LowestPrio = 1,
-   dest_SMI = 2,
-   dest__reserved_1 = 3,
-   dest_NMI = 4,
-   dest_INIT = 5,
-   dest__reserved_2 = 6,
-   dest_ExtINT = 7
+dest_Fixed = 0,
+dest_LowestPrio = 1,
+dest_SMI = 2,
+dest__reserved_1 = 3,
+dest_NMI = 4,
+dest_INIT = 5,
+dest__reserved_2 = 6,
+dest_ExtINT = 7
 };
 
 struct IO_APIC_route_entry {
@@ -135,28 +135,28 @@ unsigned int io_apic_gsi_base(unsigned int apic);
 
 static inline unsigned int __io_apic_read(unsigned int apic, unsigned int reg)
 {
-   *IO_APIC_BASE(apic) = reg;
-   return *(IO_APIC_BASE(apic)+4);
+*IO_APIC_BASE(apic) = reg;
+return *(IO_APIC_BASE(apic)+4);
 }
 
 static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
 {
-   if (ioapic_reg_remapped(reg))
-   return iommu_read_apic_from_ire(apic, reg);
-   return __io_apic_read(apic, reg);
+if ( ioapic_reg_remapped(reg) )
+return iommu_read_apic_from_ire(apic, reg);
+return __io_apic_read(apic, reg);
 }
 
 static inline void __io_apic_write(unsigned int apic, unsigned int reg, 
unsigned int value)
 {
-   *IO_APIC_BASE(apic) = reg;
-   *(IO_APIC_BASE(apic)+4) = value;
+*IO_APIC_BASE(apic) = reg;
+*(IO_APIC_BASE(apic)+4) = value;
 }
 
 static inline void io_apic_write(unsigned int apic, unsigned int reg, unsigned 
int value)
 {
-   if (ioapic_reg_remapped(reg))
-   return iommu_update_ire_from_apic(apic, reg, value);
-   __io_apic_write(apic, reg, value);
+if ( ioapic_reg_remapped(reg) )
+return iommu_update_ire_from_apic(apic, reg, value);
+__io_apic_write(apic, reg, value);
 }
 
 /*
@@ -165,9 +165,9 @@ static inline void io_apic_write(unsigned int apic, 
unsigned int reg, unsigned i
  */
 static inline void io_apic_modify(unsigned int apic, unsigned int reg, 
unsigned int value)
 {
-   if (ioapic_reg_remapped(reg))
-   return iommu_update_ire_from_apic(apic, reg, value);
-   *(IO_APIC_BASE(apic)+4) = value;
+if ( ioapic_reg_remapped(reg) )
+return iommu_update_ire_from_apic(apic, reg, value);
+*(IO_APIC_BASE(apic)+4) = value;
 }
 
 /* 1 if "noapic" boot option passed */
-- 
2.11.0

[libvirt test] 152482: regressions - FAIL

[osstest service owner]

flight 152482 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/152482/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777
 build-arm64-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-armhf-libvirt   6 libvirt-buildfail REGR. vs. 151777

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-pair  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a

version targeted for testing:
 libvirt  ed8d1385f775e8dc22887f0a1bdc425ab8e1b223
baseline version:
 libvirt  2c846fa6bcc11929c9fb857a22430fb9945654ad

Last test of basis   151777  2020-07-10 04:19:19 Z   26 days
Failing since151818  2020-07-11 04:18:52 Z   25 days   26 attempts
Testing same since   152482  2020-08-05 04:22:54 Z0 days1 attempts


People who touched revisions under test:
  Andrea Bolognani 
  Balázs Meskó 
  Bastien Orivel 
  Bihong Yu 
  Binfeng Wu 
  Boris Fiuczynski 
  Côme Borsoi 
  Daniel Henrique Barboza 
  Daniel P. Berrange 
  Daniel P. Berrangé 
  Erik Skultety 
  Fedora Weblate Translation 
  Han Han 
  Hao Wang 
  Jean-Baptiste Holcroft 
  Jianan Gao 
  Jin Yan 
  Jiri Denemark 
  Ján Tomko 
  Laine Stump 
  Liao Pingfang 
  Martin Kletzander 
  Michal Privoznik 
  Nikolay Shirokovskiy 
  Paulo de Rezende Pinatti 
  Pavel Hrdina 
  Peter Krempa 
  Pino Toscano 
  Pino Toscano 
  Piotr Drąg 
  Prathamesh Chavan 
  Roman Bogorodskiy 
  Ryan Schmidt 
  Stefan Berger 
  Szymon Scholz 
  Wang Xin 
  Weblate 
  Yang Hang 
  Yi Wang 
  Yuri Chornoivan 
  Zheng Chuan 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  fail
 build-arm64-libvirt  fail
 build-armhf-libvirt  fail
 build-i386-libvirt   fail
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   blocked 
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmblocked 
 test-amd64-amd64-libvirt-xsm blocked 
 test-arm64-arm64-libvirt-xsm blocked 
 test-amd64-i386-libvirt-xsm  blocked 
 test-amd64-amd64-libvirt blocked 
 test-arm64-arm64-libvirt blocked 
 test-armhf-armhf-libvirt blocked 
 test-amd64-i386-libvirt  blocked 
 test-amd64-amd64-libvirt-pairblocked 
 test-amd64-i386-libvirt-pair blocked 
 test-arm64-arm64-libvirt-qcow2   blocked 
 test-armhf-armhf-libvirt-raw blocked 
 test-amd64-amd64-libvirt-vhd blocked 



sg-report-flight on 

[linux-linus test] 152470: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl-xsm7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-libvirt   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-examine   8 reboot   fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-raw7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-pvshim 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-boot fail REGR. vs. 
152332
 build-arm64-pvops 6 kernel-build fail REGR. vs. 152332

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-seattle   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-thunderx  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit1   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-arm64-arm64-examine  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152332
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152332
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152332
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-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-rtds 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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 

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

[osstest service owner]

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

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  e58a71274c65e7547fc2e917f051c5c04e2820e2
baseline version:
 xen  c9f9a7258dc07735e2da2b6d0b51a0218c76a51f

Last test of basis   152477  2020-08-04 22:02:33 Z0 days
Testing same since   152487  2020-08-05 09:00:51 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

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
   c9f9a7258d..e58a71274c  e58a71274c65e7547fc2e917f051c5c04e2820e2 -> smoke

Unified Xen executable for UEFI Secure Boot support

[Trammell Hudson]

I have preliminary patches to support bundling the Xen hypervisor, xen.cfg, the 
Linux kernel, initrd and XSM into a single "unified" EFI executable that can be 
signed by sbsigntool for verification by UEFI Secure Boot.  It is inspired by 
systemd-boot's unified kernel technique and borrows the function to locate PE 
sections from systemd's LGPL'ed code.

The configuration, kernel, etc are added after building using objcopy to add 
named sections for each input file.  This allows an administrator to update the 
components independently without requiring rebuilding xen. During EFI boot, Xen 
looks at its own loaded image to locate the PE sections and, if secure boot is 
enabled, only allows use of the unified components.

The resulting EFI executable can be invoked directly from the UEFI Boot 
Manager, removing the need to use a separate loader like grub. Unlike the shim 
based verification, the signature covers the entire Xen+config+kernel+initrd 
unified file. This also ensures that properly configured platforms will measure 
the entire runtime into the TPM for unsealing secrets or remote attestation.

I've tested it on qemu OVMF with Secure Boot enabled, as well as on real 
Thinkpad hardware.  The EFI console is very slow, although it works and is able 
to boot into dom0.

The current patch set is here, and I'd appreciate suggestions on the technique 
or cleanup for submission:
https://github.com/osresearch/xen/tree/secureboot

--
Trammell

[xen-unstable-coverity test] 152488: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 xen  c9f9a7258dc07735e2da2b6d0b51a0218c76a51f
baseline version:
 xen  81fd0d3ca4b2cd309403c6e8da662c325dd35750

Last test of basis   152385  2020-08-02 09:18:50 Z3 days
Testing same since   152488  2020-08-05 09:18:39 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 
  Wei Liu 

jobs:
 coverity-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/xen.git
   81fd0d3ca4..c9f9a7258d  c9f9a7258dc07735e2da2b6d0b51a0218c76a51f -> 
coverity-tested/smoke

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Ian Jackson]

Durrant, Paul writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to 
inform the frontend via xenstore"):
> > -Original Message-
> > From: Ian Jackson 
...
> Well, I guess we address the driver domain issue in this way
> too... I will add a patch to libxl to write the libxl_device_nic mtu
> value into xenstore,

Do you mean libxl in dom0 or libxl in the driver domain ?

libxl contains code that runs in both contexts.

See device_hotplug in libxl_device.c, in particular notice
if (aodev->dev->backend_domid != domid) {

If you want the mtu to be read from the bridge, it can only be
determined by the driver domain, because the bridge is in the driver
domain.

In v2 of this series you arrange for the hotplug script to copy the
mtu from the bridge into the frontend path.  That won't work because
the hotplug script can't write to that xenstore node because (unlike a
domo0 backend) a driver domain backend doesn't have write access to
the frontend so can't create a new node there.

ISTM that it is correct that it is the hotplug script that does this
interface setup.  If it weren't for this erroneous use of the frontend
path I think the right design would be:
  * toolstack libxl reads the config file to find whether there is an MTU
  * toolstack libxl writes mtu node in backend iff one was in config
(and leaves the node absent otherwise)
  * driver domain libxl runs hotplug script
  * driver domain hotplug script looks for mtu in backend; if there
isn't one, it gets the value from the bridge and writes it to
the backend in xenstore
  * driver domain backend driver reads mtu value from backend path
  * guest domain frontend driver reads mtu value from backend path
  * on domain save/migrate, toolstack libxl will record the mtu
value as the actual configuration so that the migrated domain
will get the same mtu

I don't think I understand what (in these kind of terms) you are
proposing, in order to support the frontends that want to read the mtu
from the frontend.

>  I think the current setting of 1492 can be changed to 1500 safely
> (since nothing appears to currently use that value).

Right, that seems correct to me.

> The hotplug script should then have sufficient access to update, and
> a subsequent patch can add a mechanism to set the value from the
> config.

I think what I am missing is how this "subsequent patch" would work ?
Ie what design are we aiming for, that we are now implementing part
of ?

Ian.

Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y

[Ian Jackson]

Nick Rosbrook writes ("Re: [PATCH] libxl: avoid golang building without 
CONFIG_GOLANG=y"):
> Jan - is the problem specifically that a fresh clone,  or `git
> checkout`, etc. changes file timestamps in a way that triggers make to
> rebuild those targets? I have not used the move-if-changed approach
> before, but AFAICT that would be sufficient.

I don't think there is, from the point of view of the build system,
anything different about gengotypes than about any other in-tree
committed file which is updated using makefile rules based on only
other in-tree files and common utilities (eg, in this case, Python).

I guess using move-if-changed will probably fix this.

Jan: the reasons why this output file has to be committed are
complicated.  We've discussed them at length.  Ultimately the reason
is deliberate deficiencies[1] in golang.  Sadly this is the best of a
not-very-good set of options.

Ian.

[1] This is an extreme phrase, but justified I think.  The golang
designers have deliberately aimed at what they regard as "simplicity"
and one of the things they have "simplified" away (in their language
and in their package management and build tools) is the ability to
conveniently generate golang code at build time.  Committing the
generated code is the norm in the golang community.

Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op

[Julien Grall]

Hi,

On 05/08/2020 00:22, Stefano Stabellini wrote:

On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch adds ability to the device emulator to notify otherend
(some entity running in the guest) using a SPI and implements Arm
specific bits for it. Proposed interface allows emulator to set
the logical level of a one of a domain's IRQ lines.

Please note, this is a split/cleanup of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Signed-off-by: Julien Grall 
Signed-off-by: Oleksandr Tyshchenko 
---
  tools/libs/devicemodel/core.c   | 18 ++
  tools/libs/devicemodel/include/xendevicemodel.h |  4 
  tools/libs/devicemodel/libxendevicemodel.map|  1 +
  xen/arch/arm/dm.c   | 22 +-
  xen/common/hvm/dm.c |  1 +
  xen/include/public/hvm/dm_op.h  | 15 +++
  6 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 4d40639..30bd79f 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -430,6 +430,24 @@ int xendevicemodel_set_isa_irq_level(
  return xendevicemodel_op(dmod, domid, 1, , sizeof(op));
  }
  
+int xendevicemodel_set_irq_level(

+xendevicemodel_handle *dmod, domid_t domid, uint32_t irq,
+unsigned int level)


It is a pity that having xen_dm_op_set_pci_intx_level and
xen_dm_op_set_isa_irq_level already we need to add a third one, but from
the names alone I don't think we can reuse either of them.


The problem is not the name...



It is very similar to set_isa_irq_level. We could almost rename
xendevicemodel_set_isa_irq_level to xendevicemodel_set_irq_level or,
better, just add an alias to it so that xendevicemodel_set_irq_level is
implemented by calling xendevicemodel_set_isa_irq_level. Honestly I am
not sure if it is worth doing it though. Any other opinions?


... the problem is the interrupt field is only 8-bit. So we would only 
be able to cover IRQ 0 - 255.


It is not entirely clear how the existing subop could be extended 
without breaking existing callers.





But I think we should plan for not needing two calls (one to set level
to 1, and one to set it to 0):
https://marc.info/?l=xen-devel=159535112027405


I am not sure to understand your suggestion here? Are you suggesting to 
remove the 'level' parameter?


Cheers,

--
Julien Grall

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Julien Grall]

Hi Stefano,

On 05/08/2020 00:22, Stefano Stabellini wrote:

On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

This patch makes possible to forward Guest MMIO accesses
to a device emulator on Arm and enables that support for
Arm64.

Also update XSM code a bit to let DM op be used on Arm.
New arch DM op will be introduced in the follow-up patch.

Please note, at the moment build on Arm32 is broken
(see cmpxchg usage in hvm_send_buffered_ioreq()) if someone


Speaking of buffered_ioreq, if I recall correctly, they were only used
for VGA-related things on x86. It looks like it is still true.

If so, do we need it on ARM? Note that I don't think we can get rid of
it from the interface as it is baked into ioreq, but it might be
possible to have a dummy implementation on ARM. Or maybe not: looking at
xen/common/hvm/ioreq.c it looks like it would be difficult to
disentangle bufioreq stuff from the rest of the code.


We possibly don't need it right now. However, this could possibly be 
used in the future (e.g. virtio notification doorbell).



@@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
   */
  void leave_hypervisor_to_guest(void)
  {
+#ifdef CONFIG_IOREQ_SERVER
+/*
+ * XXX: Check the return. Shall we call that in
+ * continue_running and context_switch instead?
+ * The benefits would be to avoid calling
+ * handle_hvm_io_completion on every return.
+ */


Yeah, that could be a simple and good optimization


Well, it is not simple as it is sounds :). handle_hvm_io_completion() is 
the function in charge to mark the vCPU as waiting for I/O. So we would 
at least need to split the function.


I wrote this TODO because I wasn't sure about the complexity of 
handle_hvm_io_completion(current). Looking at it again, the main 
complexity is the looping over the IOREQ servers.


I think it would be better to optimize handle_hvm_io_completion() rather 
than trying to hack the context_switch() or continue_running().


[...]


diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5fdb6e8..5823f11 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -385,10 +385,11 @@ static inline int set_foreign_p2m_entry(struct domain *d, 
unsigned long gfn,
  mfn_t mfn)
  {
  /*
- * NOTE: If this is implemented then proper reference counting of
- *   foreign entries will need to be implemented.
+ * XXX: handle properly reference. It looks like the page may not always
+ * belong to d.


Just as a reference, and without taking away anything from the comment,
I think that QEMU is doing its own internal reference counting for these
mappings.


I am not sure how this matters here? We can't really trust the DM to do 
the right thing if it is not running in dom0.


But, IIRC, the problem is some of the pages doesn't belong to do a 
domain, so it is not possible to treat them as foreign mapping (e.g. you 
wouldn't be able to grab a reference). This investigation was done a 
couple of years ago, so this may have changed in recent Xen.


As a side note, I am a bit surprised to see most of my original TODOs 
present in the code. What is the plan to solve them?


Cheers,

--
Julien Grall

RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to inform the frontend via xenstore

[Ian Jackson]

Paul Durrant writes ("RE: [PATCH v2 4/4] tools/hotplug: modify set_mtu() to 
inform the frontend via xenstore"):
> > -Original Message-
> > From: Ian Jackson 
...
> > Actually.
> > 
> > This shouldn't be in the frontend at all, should it ?  In general the
> > backend writes to the backend and the frontend to the frontend.
> > 
> > So maybe I need to take back my R-b of
> >   [PATCH v2 3/4] public/io/netif: specify MTU override node
> > 
> > Sorry for the confusion.  I seem rather undercaffienated today.
> 
> Too late. The xenstore node has been used by Windows frontends for the best 
> part of a decade so we can't practically change the
> path. Another way would be to also modify netback to simply echo the value 
> from backend into frontend, but that seems rather
> pointless.

Hmm.  How does this interact with driver domains ?  I think a driver
domain might not have write access to this node.

Is there a value we can store in it that won't break these Windows
frontends, that libxl in the toolstack domain could write, before the
hotplug script runs in the driver domain ?

> Interestingly libxl does define an 'mtu' field for libxl_device_nic, which it 
> sets to 1492 in libxl__device_nic_setdefault() but
> never writes it into xenstore. There is even a comment:
> 
> /* nic->mtu = */
> 
> in libxl__nic_from_xenstore() which implies it should have been there, but 
> isn't.
> I still think picking up the MTU from the bridge is the better way though. 

I agree that the default should come from the bridge.  Ideally there
would be a way to override it in the config.

Thanks,
Ian.

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Julien Grall]

Hi,

On 04/08/2020 20:11, Stefano Stabellini wrote:

On Tue, 4 Aug 2020, Julien Grall wrote:

On 04/08/2020 12:10, Oleksandr wrote:

On 04.08.20 10:45, Paul Durrant wrote:

+static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
+{
+    return ioreq->state == STATE_IOREQ_READY &&
+   !ioreq->data_is_ptr &&
+   (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir !=
IOREQ_WRITE);
+}

I don't think having this in common code is correct. The short-cut of not
completing PIO reads seems somewhat x86 specific.


Hmmm, looking at the code, I think it doesn't wait for PIO writes to complete
(not read). Did I miss anything?


Does ARM even

have the concept of PIO?


I am not 100% sure here, but it seems that doesn't have.


Technically, the PIOs exist on Arm, however they are accessed the same way as
MMIO and will have a dedicated area defined by the HW.

AFAICT, on Arm64, they are only used for PCI IO Bar.

Now the question is whether we want to expose them to the Device Emulator as
PIO or MMIO access. From a generic PoV, a DM shouldn't have to care about the
architecture used. It should just be able to request a given IOport region.

So it may make sense to differentiate them in the common ioreq code as well.

I had a quick look at QEMU and wasn't able to tell if PIOs and MMIOs address
space are different on Arm as well. Paul, Stefano, do you know what they are
doing?


On the QEMU side, it looks like PIO (address_space_io) is used in
connection with the emulation of the "in" or "out" instructions, see
ioport.c:cpu_inb for instance. Some parts of PCI on QEMU emulate PIO
space regardless of the architecture, such as
hw/pci/pci_bridge.c:pci_bridge_initfn.

However, because there is no "in" and "out" on ARM, I don't think
address_space_io can be accessed. Specifically, there is no equivalent
for target/i386/misc_helper.c:helper_inb on ARM.


So how PCI I/O BAR are accessed? Surely, they could be used on Arm, right?

Cheers,

--
Julien Grall

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Jan Beulich]

On 05.08.2020 01:22, Stefano Stabellini wrote:
> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -385,10 +385,11 @@ static inline int set_foreign_p2m_entry(struct domain 
>> *d, unsigned long gfn,
>>  mfn_t mfn)
>>  {
>>  /*
>> - * NOTE: If this is implemented then proper reference counting of
>> - *   foreign entries will need to be implemented.
>> + * XXX: handle properly reference. It looks like the page may not always
>> + * belong to d.
> 
> Just as a reference, and without taking away anything from the comment,
> I think that QEMU is doing its own internal reference counting for these
> mappings.

Which of course in no way replaces the need to do proper ref counting
in Xen. (Just FAOD, as I'm not sure why you've said what you've said.)

Jan

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Jan Beulich]

On 04.08.2020 21:11, Stefano Stabellini wrote:
>> The point of the check isn't to determine whether to wait, but
>> what to do after having waited. Reads need a retry round through
>> the emulator (to store the result in the designated place),
>> while writes don't have such a requirement (and hence guest
>> execution can continue immediately in the general case).
> 
> The x86 code looks like this:
> 
> rc = hvm_send_ioreq(s, , 0);
> if ( rc != X86EMUL_RETRY || currd->is_shutting_down )
> vio->io_req.state = STATE_IOREQ_NONE;
> else if ( !hvm_ioreq_needs_completion(>io_req) )
> rc = X86EMUL_OKAY;
> 
> Basically hvm_send_ioreq is expected to return RETRY.
> Then, if it is a PIO write operation only, it is turned into OKAY right
> away. Otherwise, rc stays as RETRY.
> 
> So, normally, hvmemul_do_io is expected to return RETRY, because the
> emulator is not done yet. Am I understanding the code correctly?

"The emulator" unfortunately is ambiguous here: Do you mean qemu
(or whichever else ioreq server) or the x86 emulator inside Xen?
There are various conditions leading to RETRY. As far as
hvm_send_ioreq() goes, it is expected to return RETRY whenever
some sort of response is to be expected (the most notable
exception being the hvm_send_buffered_ioreq() path), or when
submitting the request isn't possible in the first place.

> If so, who is handling RETRY on x86? It tried to follow the call chain
> but ended up in the x86 emulator and got lost :-)

Not sure I understand the question correctly, but I'll try an
answer nevertheless: hvm_send_ioreq() arranges for the vCPU to be
put to sleep (prepare_wait_on_xen_event_channel()). Once the event
channel got signaled (and vCPU unblocked), hvm_do_resume() ->
handle_hvm_io_completion() -> hvm_wait_for_io() then check whether
the wait reason has been satisfied (wait_on_xen_event_channel()),
and ...

> At some point later, after the emulator (QEMU) has completed the
> request, handle_hvm_io_completion gets called which ends up calling
> handle_mmio() finishing the job on the Xen side too.

..., as you say, handle_hvm_io_completion() invokes the retry of
the original operation (handle_mmio() or handle_pio() in
particular) if need be.

What's potentially confusing is that there's a second form of
retry, invoked by the x86 insn emulator itself when it needs to
split complex insns (the repeated string insns being the most
important example). This results in actually exiting back to guest
context without having advanced rIP, but after having updated
other register state suitably (to express the progress made so
far).

Jan

Re: [PATCH] libxl: avoid golang building without CONFIG_GOLANG=y

[Jan Beulich]

On 04.08.2020 18:41, Nick Rosbrook wrote:
> On Tue, Aug 4, 2020 at 12:02 PM Jan Beulich  wrote:
>>
>> On 04.08.2020 17:57, Wei Liu wrote:
>>> On Tue, Aug 04, 2020 at 05:53:49PM +0200, Jan Beulich wrote:
 On 04.08.2020 17:50, Wei Liu wrote:
> On Tue, Aug 04, 2020 at 05:30:40PM +0200, Jan Beulich wrote:
>> On 04.08.2020 17:22, Nick Rosbrook wrote:
>>> On Tue, Aug 4, 2020 at 10:17 AM Wei Liu  wrote:

 On Mon, Aug 03, 2020 at 10:06:32AM +0200, Jan Beulich wrote:
> While this doesn't address the real problem I've run into (attempting 
> to
> update r/o source files), not recursing into tools/golang/xenlight/ is
> enough to fix the build for me for the moment. I don't currently see 
> why
> 60db5da62ac0 ("libxl: Generate golang bindings in libxl Makefile") 
> found
> it necessary to invoke this build step unconditionally.
>

 Perhaps an oversight?
>>>
>>> This is intentional, and I think the commit message in 60db5da62ac0
>>> ("libxl: Generate golang bindings in libxl Makefile") explains the
>>> reasoning well. But, to summarize, CONFIG_GOLANG is only used to
>>> control the bindings actually being compiled (i.e. with `go build`).
>>> However, we always want the code generation script
>>> (tools/golang/xenlight/gengotypes.py) to run if e.g.
>>> tools/libxl/libxl_types.idl is modified.
>>>
>>> I hope this helps.
>>
>> Not really - I'm still not seeing the "why" behind this behavior. I.e.
>> why build _anything_ that's not used further in the build, nor getting
>> installed? Also if (aiui) you effectively object to the change that
>> Wei has given his ack for, would you mind providing an alternative fix
>> for the problem at hand?
>
> Is the solution here to make the target check if IDL definition file is
> actually changed before regenerating the bindings?

 I don't know - Nick? A move-if-changed based approach would likely deal
 with the r/o source problem at the same time (at least until such time
 where the directory containing the file(s) is also r/o).
>>>
>>> To make sure Nick and I understand your use case correct -- "r/o source
>>> problem" means you want the tools source to be read-only? But you would
>>> be fine recursing into tools directory to build all the libraries and
>>> programs?
>>
>> Yes - until we support out-of-tree builds, nothing more can be expected
>> to work.
>>
> 
> Jan - is the problem specifically that a fresh clone,  or `git
> checkout`, etc. changes file timestamps in a way that triggers make to
> rebuild those targets?

Afaict it's not deterministic in which order files get created / updated,
and hence the generated files may or may not appear older than their
dependencies.

Jan