Re: [PATCH v1 04/10] hw/arm: xenpvh: Add support for SMP guests

[Jason Andryuk]

On 2024-08-16 12:53, Stefano Stabellini wrote:

On Fri, 16 Aug 2024, Edgar E. Iglesias wrote:

On Thu, Aug 15, 2024 at 2:30 AM Stefano Stabellini  
wrote:
   On Wed, 14 Aug 2024, Edgar E. Iglesias wrote:
   > On Tue, Aug 13, 2024 at 03:52:32PM -0700, Stefano Stabellini wrote:
   > > On Tue, 13 Aug 2024, Edgar E. Iglesias wrote:
   > > > On Mon, Aug 12, 2024 at 06:47:17PM -0700, Stefano Stabellini wrote:
   > > > > On Mon, 12 Aug 2024, Edgar E. Iglesias wrote:
   > > > > > From: "Edgar E. Iglesias" 
   > > > > >
   > > > > > Add SMP support for Xen PVH ARM guests. Create max_cpus ioreq
   > > > > > servers to handle hotplug.
   > > > > >
   > > > > > Signed-off-by: Edgar E. Iglesias 
   > > > > > ---
   > > > > >  hw/arm/xen_arm.c | 5 +++--
   > > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
   > > > > >
   > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
   > > > > > index 5f75cc3779..ef8315969c 100644
   > > > > > --- a/hw/arm/xen_arm.c
   > > > > > +++ b/hw/arm/xen_arm.c
   > > > > > @@ -173,7 +173,7 @@ static void xen_arm_init(MachineState 
*machine)
   > > > > >
   > > > > >      xen_init_ram(machine);
   > > > > >
   > > > > > -    xen_register_ioreq(xam->state, machine->smp.cpus, 
&xen_memory_listener);
   > > > > > +    xen_register_ioreq(xam->state, machine->smp.max_cpus, 
&xen_memory_listener);
   > > > > >
   > > > > >      xen_create_virtio_mmio_devices(xam);
   > > > > >
   > > > > > @@ -218,7 +218,8 @@ static void 
xen_arm_machine_class_init(ObjectClass *oc, void *data)
   > > > > >      MachineClass *mc = MACHINE_CLASS(oc);
   > > > > >      mc->desc = "Xen PVH ARM machine";
   > > > > >      mc->init = xen_arm_init;
   > > > > > -    mc->max_cpus = 1;
   > > > > > +    /* MAX number of vcpus supported by Xen.  */
   > > > > > +    mc->max_cpus = GUEST_MAX_VCPUS;
   > > > >
   > > > > Will this cause allocations of data structures with 128 elements?
   > > > > Looking at hw/xen/xen-hvm-common.c:xen_do_ioreq_register it seems
   > > > > possible? Or hw/xen/xen-hvm-common.c:xen_do_ioreq_register is 
called
   > > >
   > > > Yes, in theory there's probably overhead with this but as you 
correctly
   > > > noted below, a PVH aware xl will set the max_cpus option to a 
lower value.
   > > >
   > > > With a non-pvh aware xl, I was a little worried about the overhead
   > > > but I couldn't see any visible slow-down on ARM neither in boot or 
in network
   > > > performance (I didn't run very sophisticated benchmarks).
   > >
   > > What do you mean by "non-pvh aware xl"? All useful versions of xl
   > > support pvh?
   >
   >
   > I mean an xl without our PVH patches merged.
   > xl in upstream doesn't know much about PVH yet.
   > Even for ARM, we're still carrying significant patches in our tree.

   Oh I see. In that case, I don't think we need to support "non-pvh aware 
xl".


   > > > > later on with the precise vCPU value which should be provided to 
QEMU
   > > > > via the -smp command line option
   > > > > (tools/libs/light/libxl_dm.c:libxl__build_device_model_args_new)?
   > > >
   > > > Yes, a pvh aware xl will for example pass -smp 2,maxcpus=4 based on
   > > > values from the xl.cfg. If the user doesn't set maxvcpus in 
xl.cfg, xl
   > > > will set maxvcpus to the same value as vcpus.
   > >
   > > OK good. In that case if this is just an initial value meant to be
   > > overwritten, I think it is best to keep it as 1.
   >
   > Sorry but that won't work. I think the confusion here may be that
   > it's easy to mix up mc->max_cpus and machine->smp.max_cpus, these are
   > not the same. They have different purposes.
   >
   > I'll try to clarify the 3 values in play.
   >
   > machine-smp.cpus:
   > Number of guest vcpus active at boot.
   > Passed to QEMU via the -smp command-line option.
   > We don't use this value in QEMU's ARM PVH machines.
   >
   > machine->smp.max_cpus:
   > Max number of vcpus that the guest can use (equal or larger than 
machine-smp.cpus).
   > Will be set by xl via the "-smp X,maxcpus=Y" command-line option to 
QEMU.
   > Taken from maxvcpus from xl.cfg, same as XEN_DMOP_nr_vcpus.
   > This is what we use for xen_register_ioreq().
   >
   > mc->max_cpus:
   > Absolute MAX in QEMU used to cap the -smp command-line options.
   > If xl tries to set -smp (machine->smp.max_cpus) larger than this, QEMU 
will bail out.
   > Used to setup xen_register_ioreq() ONLY if -smp maxcpus was NOT set 
(i.e by a non PVH aware xl).
   > Cannot be 1 because that would limit QEMU to MAX 1 vcpu.
   >
   > I guess we could set mc->max_cpus to what XEN_DMOP_nr_vcpus returns 
but I'll
   > have to check if

Re: [PATCH v2 2/2] xen: fix stubdom PCI addr

[Jason Andryuk]

On Tue, Mar 5, 2024 at 2:13 PM Marek Marczykowski-Górecki
 wrote:
>
> From: Frédéric Pierret (fepitre) 

Needs to be changed to Marek.

> When running in a stubdomain, the config space access via sysfs needs to
> use BDF as seen inside stubdomain (connected via xen-pcifront), which is
> different from the real BDF. For other purposes (hypercall parameters
> etc), the real BDF needs to be used.
> Get the in-stubdomain BDF by looking up relevant PV PCI xenstore
> entries.
>
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
> Changes in v2:
> - use xs_node_scanf
> - use %d instead of %u to read values written as %d
> - add a comment from another iteration of this patch by Jason Andryuk
> ---
>  hw/xen/xen-host-pci-device.c | 69 +++-
>  hw/xen/xen-host-pci-device.h |  6 
>  2 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 8c6e9a1716..8ea2a5a4af 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -9,6 +9,8 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
> +#include "hw/xen/xen-legacy-backend.h"
> +#include "hw/xen/xen-bus-helper.h"
>  #include "xen-host-pci-device.h"
>
>  #define XEN_HOST_PCI_MAX_EXT_CAP \
> @@ -33,13 +35,67 @@
>  #define IORESOURCE_PREFETCH 0x1000  /* No side effects */
>  #define IORESOURCE_MEM_64   0x0010
>
> +/*
> + * Non-passthrough (dom0) accesses are local PCI devices and use the given 
> BDF
> + * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
> + * either have a BDF identical to the backend's BDF 
> (xen-backend.passthrough=1)
> + * or a local virtual BDF (xen-backend.passthrough=0)
> + *
> + * We are always given the backend's BDF and need to lookup the appropriate
> + * local BDF for sysfs access.
> + */
> +static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
> +{
> +unsigned int num_devs, len, i;
> +unsigned int domain, bus, dev, func;
> +char *be_path = NULL;
> +char path[80];

path is now only used for dev/vdev-%d, so 80 could be reduced.

> +
> +be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
> +if (!be_path)

error_setg() here?

> +goto out;
> +
> +if (xs_node_scanf(xenstore, 0, be_path, "num_devs", NULL, "%d", 
> &num_devs) != 1) {
> +error_setg(errp, "Failed to read or parse %s/num_devs\n", be_path);
> +goto out;
> +}
> +
> +for (i = 0; i < num_devs; i++) {
> +snprintf(path, sizeof(path), "dev-%d", i);
> +if (xs_node_scanf(xenstore, 0, be_path, path, NULL,
> +  "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
> +error_setg(errp, "Failed to read or parse %s/%s\n", be_path, 
> path);
> +goto out;
> +}
> +if (domain != d->domain ||
> +bus != d->bus ||
> +dev != d->dev ||
> +func!= d->func)
> +continue;
> +snprintf(path, sizeof(path), "vdev-%d", i);
> +if (xs_node_scanf(xenstore, 0, be_path, path, NULL,
> +  "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
> +error_setg(errp, "Failed to read or parse %s/%s\n", be_path, 
> path);
> +goto out;
> +}
> +d->local_domain = domain;
> +d->local_bus = bus;
> +d->local_dev = dev;
> +d->local_func = func;
> +goto out;
> +}

error_setg here in case we exited the loop without finding a match?

Thanks,
Jason

> +
> +out:
> +free(be_path);
> +}
> +

Re: [PATCH v2 1/2] hw/xen: detect when running inside stubdomain

[Jason Andryuk]

On Tue, Mar 5, 2024 at 2:13 PM Marek Marczykowski-Górecki
 wrote:
>
> Introduce global xen_is_stubdomain variable when qemu is running inside
> a stubdomain instead of dom0. This will be relevant for subsequent
> patches, as few things like accessing PCI config space need to be done
> differently.
>
> Signed-off-by: Marek Marczykowski-Górecki 

Reviewed-by: Jason Andryuk 

Re: [PATCH 1/2] hw/xen: detect when running inside stubdomain

[Jason Andryuk]

On Tue, Feb 20, 2024 at 1:50 AM Philippe Mathieu-Daudé
 wrote:
>
> On 19/2/24 19:16, Marek Marczykowski-Górecki wrote:
> > Introduce global xen_is_stubdomain variable when qemu is running inside
> > a stubdomain instead of dom0. This will be relevant for subsequent
> > patches, as few things like accessing PCI config space need to be done
> > differently.
> >
> > Signed-off-by: Marek Marczykowski-Górecki 
> > ---
> >   hw/xen/xen-legacy-backend.c | 15 +++
> >   include/hw/xen/xen.h|  1 +
> >   system/globals.c|  1 +
> >   3 files changed, 17 insertions(+)
>
>
> > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > index 37ecc91fc3..ecb89ecfc1 100644
> > --- a/include/hw/xen/xen.h
> > +++ b/include/hw/xen/xen.h
> > @@ -36,6 +36,7 @@ enum xen_mode {
> >   extern uint32_t xen_domid;
> >   extern enum xen_mode xen_mode;
> >   extern bool xen_domid_restrict;
> > +extern bool xen_is_stubdomain;
> >
> >   int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
> >   int xen_set_pci_link_route(uint8_t link, uint8_t irq);
> > diff --git a/system/globals.c b/system/globals.c
> > index b6d4e72530..ac27d88bd4 100644
> > --- a/system/globals.c
> > +++ b/system/globals.c
> > @@ -62,6 +62,7 @@ bool qemu_uuid_set;
> >   uint32_t xen_domid;
> >   enum xen_mode xen_mode = XEN_DISABLED;
> >   bool xen_domid_restrict;
> > +bool xen_is_stubdomain;
>
> Note for myself, Paolo and Claudio, IIUC these fields belong
> to TYPE_XEN_ACCEL in accel/xen/xen-all.c. Maybe resulting in
> smth like:

I think some of these are used by the KVM Xen-emulation, so they can't
just be moved into the Xen accelerator.  David?

Regards,
Jason

Re: [PATCH 1/2] hw/xen: detect when running inside stubdomain

[Jason Andryuk]

On Mon, Feb 19, 2024 at 1:17 PM Marek Marczykowski-Górecki
 wrote:
>
> Introduce global xen_is_stubdomain variable when qemu is running inside
> a stubdomain instead of dom0. This will be relevant for subsequent
> patches, as few things like accessing PCI config space need to be done
> differently.
>
> Signed-off-by: Marek Marczykowski-Górecki 

Reviewed-by: Jason Andryuk 

Thanks,
Jason

Re: [PATCH 2/2] xen: fix stubdom PCI addr

[Jason Andryuk]

On Mon, Feb 19, 2024 at 1:49 PM Marek Marczykowski-Górecki
 wrote:
>
> From: Frédéric Pierret (fepitre) 
>
> When running in a stubdomain, the config space access via sysfs needs to
> use BDF as seen inside stubdomain (connected via xen-pcifront), which is
> different from the real BDF. For other purposes (hypercall parameters
> etc), the real BDF needs to be used.
> Get the in-stubdomain BDF by looking up relevant PV PCI xenstore
> entries.
>
> Signed-off-by: Marek Marczykowski-Górecki 

Anthony made these comments on a different version of this patch:
https://lore.kernel.org/xen-devel/48c55d33-aa16-4867-a477-f6df45c7d9d9@perard/

(Sorry I lost track of addressing them at the time.)

Regards,
Jason

Re: [PATCH] xen: Fix host pci for stubdom

[Jason Andryuk]

On Mon, May 15, 2023 at 11:04 AM Anthony PERARD
 wrote:
>
> On Sun, Mar 19, 2023 at 08:05:54PM -0400, Jason Andryuk wrote:
> > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> > index 8c6e9a1716..51a72b432d 100644
> > --- a/hw/xen/xen-host-pci-device.c
> > +++ b/hw/xen/xen-host-pci-device.c
> > @@ -33,13 +34,101 @@
> >  #define IORESOURCE_PREFETCH 0x1000  /* No side effects */
> >  #define IORESOURCE_MEM_64   0x0010
> >
> > +/*
> > + * Non-passthrough (dom0) accesses are local PCI devices and use the given 
> > BDF
> > + * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
> > + * either have a BDF identical to the backend's BFD 
> > (xen-backend.passthrough=1)
> > + * or a local virtual BDF (xen-backend.passthrough=0)
> > + *
> > + * We are always given the backend's BDF and need to lookup the appropriate
> > + * local BDF for sysfs access.
> > + */
> > +static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
> > +{
> > +unsigned int num_devs, len, i;
> > +unsigned int domain, bus, dev, func;
> > +char *be_path;
> > +char path[80];
> > +char *msg;
> > +
> > +be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
> > +if (!be_path) {
> > +/*
> > + * be_path doesn't exist, so we are dealing with a local
> > + * (non-passthough) device.
> > + */
> > +d->local_domain = d->domain;
> > +d->local_bus = d->bus;
> > +d->local_dev = d->dev;
> > +d->local_func = d->func;
> > +
> > +return;
> > +}
> > +
> > +snprintf(path, sizeof(path), "%s/num_devs", be_path);
>
> Is 80 bytes for `path` enough?
> What if the path is truncated due to the limit?
>
>
> There's xs_node_scanf() which might be useful. It does the error
> handling and call scanf(). But I'm not sure if it can be used here, in
> this file.

Thanks for the suggestion - I'll take a look.  Your other comments
sound good, too.

Also, for the next version, I plan to change the From: to Marek. I was
thinking of doing it earlier, but failed to do so when it was time to
send out the patch.  Most of the code is Marek's from his Qubes
stubdom repo.  My modifications were to make it work with non-stubdom
as well.

Thanks,
Jason

Re: [PATCH] 9pfs/xen: Fix segfault on shutdown

[Jason Andryuk]

On Fri, May 5, 2023 at 6:05 AM Christian Schoenebeck
 wrote:
>
> Hi Jason,
>
> as this is a Xen specific change, I would like Stefano or another Xen
> developer to take a look at it, just few things from my side ...
>
> On Tuesday, May 2, 2023 4:37:22 PM CEST Jason Andryuk wrote:
> > xen_9pfs_free can't use gnttabdev since it is already closed and NULL-ed
>
> Where exactly does it do that access? A backtrace or another detailed commit
> log description would help.

The segfault is down in the xen grant libraries during the free
callback.  The call stack is roughly:
xen_pv_del_xendev(struct XenLegacyDevice *xendev)
xen_9pfs_free() (->free() callback)
xen_be_unmap_grant_refs(&xen_9pdev->xendev, ...)
qemu_xen_gnttab_unmap(xendev->gnttabdev, ...)
xengnttab_unmap(xgt, ...) <- segfault.

The device went through the "disconnect" state before free() is
called, so xen_be_disconnect() already ran which did:
if (xendev->gnttabdev) {
qemu_xen_gnttab_close(xendev->gnttabdev);
xendev->gnttabdev = NULL;
}

gnttabdev being used by xengnttab_unmap().

> > out when free is called.  Do the teardown in _disconnect().  This
> > matches the setup done in _connect().
> >
> > trace-events are also added for the XenDevOps functions.
> >
> > Signed-off-by: Jason Andryuk 
> > ---
> >  hw/9pfs/trace-events |  5 +
> >  hw/9pfs/xen-9p-backend.c | 36 +++-
> >  2 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
> > index 6c77966c0b..7b5b0b5a48 100644
> > --- a/hw/9pfs/trace-events
> > +++ b/hw/9pfs/trace-events
> > @@ -48,3 +48,8 @@ v9fs_readlink(uint16_t tag, uint8_t id, int32_t fid) "tag 
> > %d id %d fid %d"
> >  v9fs_readlink_return(uint16_t tag, uint8_t id, char* target) "tag %d id %d 
> > name %s"
> >  v9fs_setattr(uint16_t tag, uint8_t id, int32_t fid, int32_t valid, int32_t 
> > mode, int32_t uid, int32_t gid, int64_t size, int64_t atime_sec, int64_t 
> > mtime_sec) "tag %u id %u fid %d iattr={valid %d mode %d uid %d gid %d size 
> > %"PRId64" atime=%"PRId64" mtime=%"PRId64" }"
> >  v9fs_setattr_return(uint16_t tag, uint8_t id) "tag %u id %u"
> > +
>
> Nit-picking; missing leading comment:
>
> # xen-9p-backend.c

Will do, thanks.

> > +xen_9pfs_alloc(char *name) "name %s"
> > +xen_9pfs_connect(char *name) "name %s"
> > +xen_9pfs_disconnect(char *name) "name %s"
> > +xen_9pfs_free(char *name) "name %s"
> > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > index 0e266c552b..c646a0b3d1 100644
> > --- a/hw/9pfs/xen-9p-backend.c
> > +++ b/hw/9pfs/xen-9p-backend.c
> > @@ -25,6 +25,8 @@
> >  #include "qemu/iov.h"
> >  #include "fsdev/qemu-fsdev.h"
> >
> > +#include "trace.h"
> > +
> >  #define VERSIONS "1"
> >  #define MAX_RINGS 8
> >  #define MAX_RING_ORDER 9
> > @@ -337,6 +339,8 @@ static void xen_9pfs_disconnect(struct XenLegacyDevice 
> > *xendev)
> >  Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
> >  int i;
> >
> > +trace_xen_9pfs_disconnect(xendev->name);
> > +
> >  for (i = 0; i < xen_9pdev->num_rings; i++) {
> >  if (xen_9pdev->rings[i].evtchndev != NULL) {
> >  
> > qemu_set_fd_handler(qemu_xen_evtchn_fd(xen_9pdev->rings[i].evtchndev),
> > @@ -345,40 +349,42 @@ static void xen_9pfs_disconnect(struct 
> > XenLegacyDevice *xendev)
> > xen_9pdev->rings[i].local_port);
> >  xen_9pdev->rings[i].evtchndev = NULL;
> >  }
> > -}
> > -}
> > -
> > -static int xen_9pfs_free(struct XenLegacyDevice *xendev)
> > -{
> > -Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
> > -int i;
> > -
> > -if (xen_9pdev->rings[0].evtchndev != NULL) {
> > -xen_9pfs_disconnect(xendev);
> > -}
> > -
> > -for (i = 0; i < xen_9pdev->num_rings; i++) {
> >  if (xen_9pdev->rings[i].data != NULL) {
> >  xen_be_unmap_grant_refs(&xen_9pdev->xendev,
> >  xen_9pdev->rings[i].data,
> >  xen_9pdev->rings[i].intf->ref,
> >  (1 << xen_9pdev->rings[i].ring_order));
> > +xen_9p

[PATCH] 9pfs/xen: Fix segfault on shutdown

[Jason Andryuk]

xen_9pfs_free can't use gnttabdev since it is already closed and NULL-ed
out when free is called.  Do the teardown in _disconnect().  This
matches the setup done in _connect().

trace-events are also added for the XenDevOps functions.

Signed-off-by: Jason Andryuk 
---
 hw/9pfs/trace-events |  5 +
 hw/9pfs/xen-9p-backend.c | 36 +++-
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
index 6c77966c0b..7b5b0b5a48 100644
--- a/hw/9pfs/trace-events
+++ b/hw/9pfs/trace-events
@@ -48,3 +48,8 @@ v9fs_readlink(uint16_t tag, uint8_t id, int32_t fid) "tag %d 
id %d fid %d"
 v9fs_readlink_return(uint16_t tag, uint8_t id, char* target) "tag %d id %d 
name %s"
 v9fs_setattr(uint16_t tag, uint8_t id, int32_t fid, int32_t valid, int32_t 
mode, int32_t uid, int32_t gid, int64_t size, int64_t atime_sec, int64_t 
mtime_sec) "tag %u id %u fid %d iattr={valid %d mode %d uid %d gid %d size 
%"PRId64" atime=%"PRId64" mtime=%"PRId64" }"
 v9fs_setattr_return(uint16_t tag, uint8_t id) "tag %u id %u"
+
+xen_9pfs_alloc(char *name) "name %s"
+xen_9pfs_connect(char *name) "name %s"
+xen_9pfs_disconnect(char *name) "name %s"
+xen_9pfs_free(char *name) "name %s"
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 0e266c552b..c646a0b3d1 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -25,6 +25,8 @@
 #include "qemu/iov.h"
 #include "fsdev/qemu-fsdev.h"
 
+#include "trace.h"
+
 #define VERSIONS "1"
 #define MAX_RINGS 8
 #define MAX_RING_ORDER 9
@@ -337,6 +339,8 @@ static void xen_9pfs_disconnect(struct XenLegacyDevice 
*xendev)
 Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
 int i;
 
+trace_xen_9pfs_disconnect(xendev->name);
+
 for (i = 0; i < xen_9pdev->num_rings; i++) {
 if (xen_9pdev->rings[i].evtchndev != NULL) {
 
qemu_set_fd_handler(qemu_xen_evtchn_fd(xen_9pdev->rings[i].evtchndev),
@@ -345,40 +349,42 @@ static void xen_9pfs_disconnect(struct XenLegacyDevice 
*xendev)
xen_9pdev->rings[i].local_port);
 xen_9pdev->rings[i].evtchndev = NULL;
 }
-}
-}
-
-static int xen_9pfs_free(struct XenLegacyDevice *xendev)
-{
-Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
-int i;
-
-if (xen_9pdev->rings[0].evtchndev != NULL) {
-xen_9pfs_disconnect(xendev);
-}
-
-for (i = 0; i < xen_9pdev->num_rings; i++) {
 if (xen_9pdev->rings[i].data != NULL) {
 xen_be_unmap_grant_refs(&xen_9pdev->xendev,
 xen_9pdev->rings[i].data,
 xen_9pdev->rings[i].intf->ref,
 (1 << xen_9pdev->rings[i].ring_order));
+xen_9pdev->rings[i].data = NULL;
 }
 if (xen_9pdev->rings[i].intf != NULL) {
 xen_be_unmap_grant_ref(&xen_9pdev->xendev,
xen_9pdev->rings[i].intf,
xen_9pdev->rings[i].ref);
+xen_9pdev->rings[i].intf = NULL;
 }
 if (xen_9pdev->rings[i].bh != NULL) {
 qemu_bh_delete(xen_9pdev->rings[i].bh);
+xen_9pdev->rings[i].bh = NULL;
 }
 }
 
 g_free(xen_9pdev->id);
+xen_9pdev->id = NULL;
 g_free(xen_9pdev->tag);
+xen_9pdev->tag = NULL;
 g_free(xen_9pdev->path);
+xen_9pdev->path = NULL;
 g_free(xen_9pdev->security_model);
+xen_9pdev->security_model = NULL;
 g_free(xen_9pdev->rings);
+xen_9pdev->rings = NULL;
+return;
+}
+
+static int xen_9pfs_free(struct XenLegacyDevice *xendev)
+{
+trace_xen_9pfs_free(xendev->name);
+
 return 0;
 }
 
@@ -390,6 +396,8 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
 V9fsState *s = &xen_9pdev->state;
 QemuOpts *fsdev;
 
+trace_xen_9pfs_connect(xendev->name);
+
 if (xenstore_read_fe_int(&xen_9pdev->xendev, "num-rings",
  &xen_9pdev->num_rings) == -1 ||
 xen_9pdev->num_rings > MAX_RINGS || xen_9pdev->num_rings < 1) {
@@ -499,6 +507,8 @@ out:
 
 static void xen_9pfs_alloc(struct XenLegacyDevice *xendev)
 {
+trace_xen_9pfs_alloc(xendev->name);
+
 xenstore_write_be_str(xendev, "versions", VERSIONS);
 xenstore_write_be_int(xendev, "max-rings", MAX_RINGS);
 xenstore_write_be_int(xendev, "max-ring-page-order", MAX_RING_ORDER);
-- 
2.40.1

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Jason Andryuk]

On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD  wrote:
>
> On Sat, Apr 01, 2023 at 10:36:45PM +, Bernhard Beschow wrote:
> >
> >
> > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD 
> > :
> > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
> > >> This is a preparational patch for the next one to make the following
> > >> more obvious:
> > >>
> > >> First, pci_bus_irqs() is now called twice in case of Xen where the
> > >> second call overrides the pci_set_irq_fn with the Xen variant.
> > >
> > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
> > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
> > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
> > >call, or maybe some other way to avoid the leak?
> >
> > Thanks for catching this! I'll post a v4.
> >
> > I think the most fool-proof way to fix this is to free irq_count just 
> > before the assignment. pci_bus_irqs_cleanup() would then have to NULL the 
> > attribute such that pci_bus_irqs() can be called afterwards.
> >
> > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen 
> > guest with my pc-piix4 branch without success. This branch essentially just 
> > provides slightly different PCI IDs for PIIX. Does xl or something else in 
> > Xen check these? If not then this means I'm still missing something. Under 
> > KVM this branch works just fine. Any idea?
>
> Maybe the ACPI tables provided by libxl needs to be updated.
> Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
> id (I know that the PCI id of the root bus is checked, but I don't know
> if that's the one that's been changed).

Xen also has hvmloader, which runs before SeaBIOS/OVMF.  Looking at
tools/firmware/hvmloader/pci.c, it has
ASSERT((devfn != PCI_ISA_DEVFN) ||
   ((vendor_id == 0x8086) && (device_id == 0x7000)));

>From QEMU, it looks like 0x7000 is PCI_DEVICE_ID_INTEL_82371SB_0, but
PIIX4 uses 0x7110 (PCI_DEVICE_ID_INTEL_82371AB_0).  Maybe try removing
that check?

Regards,
Jason

Re: [PATCH] xen: Fix host pci for stubdom

[Jason Andryuk]

On Mon, Mar 20, 2023 at 2:41 PM Bernhard Beschow  wrote:
>
>
>
> Am 20. März 2023 00:05:54 UTC schrieb Jason Andryuk :
> >PCI passthrough for an HVM with a stubdom is PV PCI passthrough from
> >dom0 to the stubdom, and then QEMU passthrough of the PCI device inside
> >the stubdom.  xen-pciback has boolean module param passthrough which
> >controls "how to export PCI topology to guest".  If passthrough=1, the
> >frontend shows a PCI SBDF matching the backend host device.  When
> >passthough=0, the frontend will get a sequentially allocated SBDF.
> >
> >libxl passes the host SBDF over QMP to QEMU.  For non-stubdom or stubdom
> >with passthrough=1, this works fine.  However, it fails for
> >passthrough=0 when QEMU can't find the sysfs node for the host SBDF.
> >
> >Handle all these cases.  Look for the xenstore frontend nodes.  If they
> >are missing, then default to using the QMP command provided SBDF.  This
> >is the non-stubdom case.  If xenstore nodes are found, then read the
> >local SBDF from the xenstore nodes.  This will handle either
> >passthrough=0/1 case.
> >
> >Based on a stubdom-specific patch originally by Marek
> >Marczykowski-Górecki , which is based
> >on earlier work by HW42 
> >
> >Signed-off-by: Jason Andryuk 
> >---
> > hw/xen/xen-host-pci-device.c | 96 +++-
> > hw/xen/xen-host-pci-device.h |  6 +++
> > 2 files changed, 101 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> >index 8c6e9a1716..51a72b432d 100644
> >--- a/hw/xen/xen-host-pci-device.c
> >+++ b/hw/xen/xen-host-pci-device.c
> >@@ -9,6 +9,7 @@
> > #include "qemu/osdep.h"
> > #include "qapi/error.h"
> > #include "qemu/cutils.h"
> >+#include "hw/xen/xen-legacy-backend.h"
> > #include "xen-host-pci-device.h"
> >
> > #define XEN_HOST_PCI_MAX_EXT_CAP \
> >@@ -33,13 +34,101 @@
> > #define IORESOURCE_PREFETCH 0x1000  /* No side effects */
> > #define IORESOURCE_MEM_64   0x0010
> >
> >+/*
> >+ * Non-passthrough (dom0) accesses are local PCI devices and use the given 
> >BDF
> >+ * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
>
> I'm unable to parse this sentence, which may be due to my unfamiliarity with 
> Xen terminology.

It's two sentences, but it's missing a period.
"Non-passthrough (dom0) accesses are local PCI devices and use the
given BDF."  and "Passthough (stubdom) accesses are through PV
frontend PCI device."

> There is also an extra space before "Those".

It's two spaces between sentences, which visually separates the
sentences.  It's a common formatting, so I think it's okay.

Thanks for taking a look.

> >+ * either have a BDF identical to the backend's BFD 
> >(xen-backend.passthrough=1)

(And a typo here: s/BFD/BDF/)

> >+ * or a local virtual BDF (xen-backend.passthrough=0)
> >+ *
> >+ * We are always given the backend's BDF and need to lookup the appropriate
> >+ * local BDF for sysfs access.
> >+ */

Regards,
Jason

[PATCH] xen: Fix host pci for stubdom

[Jason Andryuk]

PCI passthrough for an HVM with a stubdom is PV PCI passthrough from
dom0 to the stubdom, and then QEMU passthrough of the PCI device inside
the stubdom.  xen-pciback has boolean module param passthrough which
controls "how to export PCI topology to guest".  If passthrough=1, the
frontend shows a PCI SBDF matching the backend host device.  When
passthough=0, the frontend will get a sequentially allocated SBDF.

libxl passes the host SBDF over QMP to QEMU.  For non-stubdom or stubdom
with passthrough=1, this works fine.  However, it fails for
passthrough=0 when QEMU can't find the sysfs node for the host SBDF.

Handle all these cases.  Look for the xenstore frontend nodes.  If they
are missing, then default to using the QMP command provided SBDF.  This
is the non-stubdom case.  If xenstore nodes are found, then read the
local SBDF from the xenstore nodes.  This will handle either
passthrough=0/1 case.

Based on a stubdom-specific patch originally by Marek
Marczykowski-Górecki , which is based
on earlier work by HW42 

Signed-off-by: Jason Andryuk 
---
 hw/xen/xen-host-pci-device.c | 96 +++-
 hw/xen/xen-host-pci-device.h |  6 +++
 2 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 8c6e9a1716..51a72b432d 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -9,6 +9,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
+#include "hw/xen/xen-legacy-backend.h"
 #include "xen-host-pci-device.h"
 
 #define XEN_HOST_PCI_MAX_EXT_CAP \
@@ -33,13 +34,101 @@
 #define IORESOURCE_PREFETCH 0x1000  /* No side effects */
 #define IORESOURCE_MEM_64   0x0010
 
+/*
+ * Non-passthrough (dom0) accesses are local PCI devices and use the given BDF
+ * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
+ * either have a BDF identical to the backend's BFD (xen-backend.passthrough=1)
+ * or a local virtual BDF (xen-backend.passthrough=0)
+ *
+ * We are always given the backend's BDF and need to lookup the appropriate
+ * local BDF for sysfs access.
+ */
+static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
+{
+unsigned int num_devs, len, i;
+unsigned int domain, bus, dev, func;
+char *be_path;
+char path[80];
+char *msg;
+
+be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
+if (!be_path) {
+/*
+ * be_path doesn't exist, so we are dealing with a local
+ * (non-passthough) device.
+ */
+d->local_domain = d->domain;
+d->local_bus = d->bus;
+d->local_dev = d->dev;
+d->local_func = d->func;
+
+return;
+}
+
+snprintf(path, sizeof(path), "%s/num_devs", be_path);
+msg = qemu_xen_xs_read(xenstore, 0, path, &len);
+if (!msg) {
+goto err_out;
+}
+
+if (sscanf(msg, "%u", &num_devs) != 1) {
+error_setg(errp, "Failed to parse %s (%s)", msg, path);
+goto err_out;
+}
+free(msg);
+
+for (i = 0; i < num_devs; i++) {
+snprintf(path, sizeof(path), "%s/dev-%u", be_path, i);
+msg = qemu_xen_xs_read(xenstore, 0, path, &len);
+if (!msg) {
+error_setg(errp, "Failed to read %s", path);
+goto err_out;
+}
+if (sscanf(msg, "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
+error_setg(errp, "Failed to parse %s (%s)", msg, path);
+goto err_out;
+}
+free(msg);
+if (domain != d->domain ||
+bus != d->bus ||
+dev != d->dev ||
+func != d->func)
+continue;
+snprintf(path, sizeof(path), "%s/vdev-%u", be_path, i);
+msg = qemu_xen_xs_read(xenstore, 0, path, &len);
+if (!msg) {
+error_setg(errp, "Failed to read %s", path);
+goto out;
+}
+if (sscanf(msg, "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
+error_setg(errp, "Failed to parse %s (%s)", msg, path);
+goto err_out;
+}
+free(msg);
+d->local_domain = domain;
+d->local_bus = bus;
+d->local_dev = dev;
+d->local_func = func;
+goto out;
+}
+
+error_setg(errp, "Failed to find local %x:%x:%x.%x", d->domain, d->bus,
+   d->dev, d->func);
+
+err_out:
+free(msg);
+out:
+free(be_path);
+}
+
 static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
 const char *name, char *buf, ssize_t size)
 {
 int rc;
 
 rc = snprintf(buf, si

Re: [PATCH] accel/xen: Fix DM state change notification in dm_restrict mode

[Jason Andryuk]

On Tue, Mar 14, 2023 at 4:35 AM David Woodhouse  wrote:
>
> From: David Woodhouse 
>
> When dm_restrict is set, QEMU isn't permitted to update the XenStore node
> to indicate its running status. Previously, the xs_write() call would fail
> but the failure was ignored.
>
> However, in refactoring to allow for emulated XenStore operations, a new
> call to xs_open() was added. That one didn't fail gracefully, causing a
> fatal error when running in dm_restrict mode.
>
> Partially revert the offending patch, removing the additional call to
> xs_open() because the global 'xenstore' variable is still available; it
> just needs to be used with qemu_xen_xs_write() now instead of directly
> with the xs_write() libxenstore function.
>
> Also make the whole thing conditional on !xen_domid_restrict. There's no
> point even registering the state change handler to attempt to update the
> XenStore node when we know it's destined to fail.
>
> Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to 
> internal emulation")
> Reported-by: Jason Andryuk 
> Co-developed-by: Jason Andryuk 
> Not-Signed-off-by: Jason Andryuk 
> Signed-off-by: David Woodhouse 
> Will-be-Tested-by: Jason Andryuk 

Signed-off-by: Jason Andryuk 
Tested-by: Jason Andryuk 

Thanks, David.

-Jason

Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation

[Jason Andryuk]

Hi, David,

On Mon, Mar 13, 2023 at 4:45 AM David Woodhouse  wrote:
>
> On Sun, 2023-03-12 at 15:19 -0400, Jason Andryuk wrote:
> >
> > This breaks dm_restrict=1 since the xs_open is not allowed by the
> > time
> > this is called.  There are other evtchn errors before this as well:
> > # cat /var/log/xen/qemu-dm-debian.log
> > char device redirected to /dev/pts/8 (label serial0)
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > xen be core: can't open evtchn device
> > Could not contact XenStore
> >
> > Ok, those "xen be core: can't open evtchn device" were there before
> > the recent changes and seem to be non-fatal.
>
> Hm, I *think* we can just revert that part and use the global
> 'xenstore' like we did before, except via the new ops.
>
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -32,28 +32,18 @@ xendevicemodel_handle *xen_dmod;
>
>  static void xenstore_record_dm_state(const char *state)
>  {
> -struct xs_handle *xs;
>  char path[50];
>
> -/* We now have everything we need to set the xenstore entry. */
> -xs = xs_open(0);
> -if (xs == NULL) {
> -fprintf(stderr, "Could not contact XenStore\n");
> -exit(1);
> -}
> -
>  snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
>  /*
>   * This call may fail when running restricted so don't make it fatal in
>   * that case. Toolstacks should instead use QMP to listen for state 
> changes.
>   */
> -if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
> +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
>  !xen_domid_restrict) {
>  error_report("error recording dm state");
>  exit(1);
>  }
> -
> -xs_close(xs);
>  }

This looks good, better than what I posted, and seems to work for both
dm_restrict set and unset.

>
> Alternatively, that xs_write is destined to fail anyway in the
> xen_domid_restrict case, isn't it? So the xs_open() should be allowed
> to fail similarly. Or perhaps we shouldn't even *try*?

For dm_restricted, xs_write() does fail.  I verified that with a print
statement.  I think "shouldn't even try" makes sense.  I'm thinking
that  xen_domid_restricted shouldn't even add the callback.  Something
like:

--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -39,8 +39,7 @@ static void xenstore_record_dm_state(const char *state)
  * This call may fail when running restricted so don't make it fatal in
  * that case. Toolstacks should instead use QMP to listen for
state changes.
  */
-if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state)) &&
-!xen_domid_restrict) {
+if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) {
 error_report("error recording dm state");
 exit(1);
 }
@@ -101,7 +100,10 @@ static int xen_init(MachineState *ms)
 xc_interface_close(xen_xc);
 return -1;
 }
-qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
+if(!xen_domid_restrict)
+qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
 /*
  * opt out of system RAM being allocated by generic code
  */

That works for both dm_restrict 0 & 1.

I think you should submit your change and I can follow up with the
above if it seems desirable.

Thanks,
Jason

[PATCH] xen: fix dm_restrict startup

[Jason Andryuk]

QEMU is failing to signal it start when launched by libxl with
dm_restrict=1.  When xenstore_record_dm_state() is called, the
restrictions prevent xs_open() from succeeding.  QEMU can't write
running to the xenstore, and libxl fails the VM start up.

Pass in a open xenstore connection.  Let the call back use it and the
close it.  Use the completed boolean to only allow it to be called once.
This lets the xenstore connection be closed after the startup
indication.

Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to 
internal emulation")
Signed-off-by: Jason Andryuk 
---
I think xenstore_record_dm_state() only needs to indicate startup once,
so the completed bool makes sense.

 accel/xen/xen-all.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 00221e23c5..3843299843 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -30,17 +30,13 @@ xc_interface *xen_xc;
 xenforeignmemory_handle *xen_fmem;
 xendevicemodel_handle *xen_dmod;
 
-static void xenstore_record_dm_state(const char *state)
+static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
 {
-struct xs_handle *xs;
+static bool completed;
 char path[50];
 
-/* We now have everything we need to set the xenstore entry. */
-xs = xs_open(0);
-if (xs == NULL) {
-fprintf(stderr, "Could not contact XenStore\n");
-exit(1);
-}
+if (completed)
+return;
 
 snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
 /*
@@ -53,6 +49,7 @@ static void xenstore_record_dm_state(const char *state)
 exit(1);
 }
 
+completed = true;
 xs_close(xs);
 }
 
@@ -60,9 +57,10 @@ static void xenstore_record_dm_state(const char *state)
 static void xen_change_state_handler(void *opaque, bool running,
  RunState state)
 {
+struct xs_handle *xs = opaque;
 if (running) {
 /* record state running */
-xenstore_record_dm_state("running");
+xenstore_record_dm_state(xs, "running");
 }
 }
 
@@ -92,6 +90,7 @@ static void xen_setup_post(MachineState *ms, AccelState 
*accel)
 static int xen_init(MachineState *ms)
 {
 MachineClass *mc = MACHINE_GET_CLASS(ms);
+struct xs_handle *xs;
 
 xen_xc = xc_interface_open(0, 0, 0);
 if (xen_xc == NULL) {
@@ -111,7 +110,14 @@ static int xen_init(MachineState *ms)
 xc_interface_close(xen_xc);
 return -1;
 }
-qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
+xs = xs_open(0);
+if (xs == NULL) {
+fprintf(stderr, "Could not contact XenStore\n");
+exit(1);
+}
+
+qemu_add_vm_change_state_handler(xen_change_state_handler, xs);
 /*
  * opt out of system RAM being allocated by generic code
  */
-- 
2.37.2

Re: [PULL 13/27] hw/xen: Add xenstore operations to allow redirection to internal emulation

[Jason Andryuk]

On Tue, Mar 7, 2023 at 1:29 PM David Woodhouse  wrote:
>
> From: Paul Durrant 
>
> Signed-off-by: Paul Durrant 
> Signed-off-by: David Woodhouse 
> Reviewed-by: Paul Durrant 
> ---
>  accel/xen/xen-all.c |  11 +-
>  hw/char/xen_console.c   |   2 +-
>  hw/i386/kvm/xen_xenstore.c  |   3 -
>  hw/i386/kvm/xenstore_impl.h |   8 +-
>  hw/xen/xen-bus-helper.c |  62 +++
>  hw/xen/xen-bus.c| 261 
>  hw/xen/xen-legacy-backend.c | 119 +++--
>  hw/xen/xen-operations.c | 198 +
>  hw/xen/xen_devconfig.c  |   4 +-
>  hw/xen/xen_pt_graphics.c|   1 -
>  hw/xen/xen_pvdev.c  |  49 +-
>  include/hw/xen/xen-bus-helper.h |  26 +--
>  include/hw/xen/xen-bus.h|  17 +-
>  include/hw/xen/xen-legacy-backend.h |   6 +-
>  include/hw/xen/xen_backend_ops.h| 163 +
>  include/hw/xen/xen_common.h |   1 -
>  include/hw/xen/xen_pvdev.h  |   2 +-
>  softmmu/globals.c   |   1 +
>  18 files changed, 525 insertions(+), 409 deletions(-)
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index e85e4aeba5..425216230f 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -90,12 +90,15 @@ void xenstore_store_pv_console_info(int i, Chardev *chr)
>  }
>
>
> -static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
> +static void xenstore_record_dm_state(const char *state)
>  {
> +struct xs_handle *xs;
>  char path[50];
>
> +/* We now have everything we need to set the xenstore entry. */
> +xs = xs_open(0);
>  if (xs == NULL) {
> -error_report("xenstore connection not initialized");
> +fprintf(stderr, "Could not contact XenStore\n");
>  exit(1);
>  }

This breaks dm_restrict=1 since the xs_open is not allowed by the time
this is called.  There are other evtchn errors before this as well:
# cat /var/log/xen/qemu-dm-debian.log
char device redirected to /dev/pts/8 (label serial0)
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
xen be core: can't open evtchn device
Could not contact XenStore

Ok, those "xen be core: can't open evtchn device" were there before
the recent changes and seem to be non-fatal.

Regards,
Jason

Re: [PATCH 2/2] Do not access /dev/mem in MSI-X PCI passthrough on Xen

[Jason Andryuk]

On Wed, Nov 16, 2022 at 10:34 PM Marek Marczykowski-Górecki
 wrote:
>
> On Wed, Nov 16, 2022 at 10:40:02PM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Nov 16, 2022 at 02:15:22PM -0500, Jason Andryuk wrote:
> > > On Mon, Nov 14, 2022 at 2:21 PM Marek Marczykowski-Górecki
> > >  wrote:
> > > >
> > > > The /dev/mem is used for two purposes:
> > > >  - reading PCI_MSIX_ENTRY_CTRL_MASKBIT
> > > >  - reading Pending Bit Array (PBA)
> > > >
> > > > The first one was originally done because when Xen did not send all
> > > > vector ctrl writes to the device model, so QEMU might have outdated old
> > > > register value. This has been changed in Xen, so QEMU can now use its
> > > > cached value of the register instead.
> > > >
> > > > The Pending Bit Array (PBA) handling is for the case where it lives on
> > > > the same page as the MSI-X table itself. Xen has been extended to handle
> > > > this case too (as well as other registers that may live on those pages),
> > > > so QEMU handling is not necessary anymore.
> > > >
> > > > Removing /dev/mem access is useful to work within stubdomain, and
> > > > necessary when dom0 kernel runs in lockdown mode.
> > > >
> > > > Signed-off-by: Marek Marczykowski-Górecki 
> > > > 
> > >
> > > I put the Xen, QEMU, and xen-pciback patches into OpenXT and gave a
> > > little test.  When pci_permissive=0, iwlwifi fails to load its
> > > firmware.  With pci_permissive=1, it looks like MSI-X is enabled. (I
> > > previously included your libxl allow_interrupt_control patch - that
> > > seemed to get regular MSIs working prior to the MSI-X patches.)  I
> > > also removed the OpenXT equivalent of 0005-Disable-MSI-X-caps.patch.
> > > I am testing with Linux 5.4.y, so that could be another factor.
> >
> > Can you confirm the allow_interrupt_control is set by libxl? Also,
> > vanilla 5.4 doesn't have the allow_interrupt_control patch at all, and you
> > may have an earlier version that had "allow_msi_enable" as the sysfs
> > file name.

I backported allow_interrupt_control to 5.4 and that is set properly.

> Ok, I found what is wrong. Enabling MSI-X is refused, because INTx isn't
> disabled at this point yet. And apparently I was testing this with
> permissive=1...
>
> Linux does this:
> https://github.com/torvalds/linux/blob/master/drivers/pci/msi/msi.c#L611
> In short:
> 1. Enable MSI-X with MASKALL=1
> 2. Setup MSI-X table
> 3. Disable INTx
> 4. Set MASKALL=0
>
> This patch on top should fix this:
> 8<
> diff --git a/drivers/xen/xen-pciback/conf_space_capability.c 
> b/drivers/xen/xen-pciback/conf_space_capability.c
> index 097316a74126..f4c4381de76e 100644
> --- a/drivers/xen/xen-pciback/conf_space_capability.c
> +++ b/drivers/xen/xen-pciback/conf_space_capability.c
> @@ -235,7 +235,7 @@ static int msi_msix_flags_write(struct pci_dev *dev, int 
> offset, u16 new_value,
> (new_value ^ old_value) & ~field_config->allowed_bits)
> return PCIBIOS_SET_FAILED;
>
> -   if (new_value & field_config->enable_bit) {
> +   if ((new_value & field_config->allowed_bits) == 
> field_config->enable_bit) {
> /* don't allow enabling together with other interrupt types */
> int int_type = xen_pcibk_get_interrupt_type(dev);
>
> 8<

FWIW, I can confirm this allows enabling MSI-X with permissive=0 for me.

Thanks,
Jason

Re: [PATCH 2/2] Do not access /dev/mem in MSI-X PCI passthrough on Xen

[Jason Andryuk]

On Mon, Nov 14, 2022 at 2:21 PM Marek Marczykowski-Górecki
 wrote:
>
> The /dev/mem is used for two purposes:
>  - reading PCI_MSIX_ENTRY_CTRL_MASKBIT
>  - reading Pending Bit Array (PBA)
>
> The first one was originally done because when Xen did not send all
> vector ctrl writes to the device model, so QEMU might have outdated old
> register value. This has been changed in Xen, so QEMU can now use its
> cached value of the register instead.
>
> The Pending Bit Array (PBA) handling is for the case where it lives on
> the same page as the MSI-X table itself. Xen has been extended to handle
> this case too (as well as other registers that may live on those pages),
> so QEMU handling is not necessary anymore.
>
> Removing /dev/mem access is useful to work within stubdomain, and
> necessary when dom0 kernel runs in lockdown mode.
>
> Signed-off-by: Marek Marczykowski-Górecki 

I put the Xen, QEMU, and xen-pciback patches into OpenXT and gave a
little test.  When pci_permissive=0, iwlwifi fails to load its
firmware.  With pci_permissive=1, it looks like MSI-X is enabled. (I
previously included your libxl allow_interrupt_control patch - that
seemed to get regular MSIs working prior to the MSI-X patches.)  I
also removed the OpenXT equivalent of 0005-Disable-MSI-X-caps.patch.
I am testing with Linux 5.4.y, so that could be another factor.

One strange thing is the lspci output.  Dom0 shows MSI-X enabled.
Meanwhile NDVM (sys-net) does not show the MSI-X capability.  If you
`hexdump -C /sys/bus/pci/devices/$dev/config` you can see MSI-X
enabled, but you also see that the MSI capability has 00 as the next
pointer, so lspci stops parsing.

MSI cap stubdom:
0040  10 00 92 00 c0 0e 00 00  10 0c 10 00 00 00 00 00  ||
0x41 -> next 0x00
MSI cap dom0:
0040  10 80 92 00 c0 0e 00 10  10 0c 10 00 00 00 00 00  ||
0x41 -> next 0x80

MSI-X:
0080  11 00 0f 80 00 20 00 00  00 30 00 00 00 00 00 00

AFAIU, the value 0x80 at offset 0x83 is MSI-X Enabled.

I had a boot where assignment failed with the hypervisor printing:
d12: assign (:00:14.3) failed (-16)
Rebooting the laptop seemed to clear that.

Regards,
Jason

Re: [PATCH] xen-hvm: Allow disabling buffer_io_timer

[Jason Andryuk]

On Tue, Dec 14, 2021 at 8:40 AM Durrant, Paul  wrote:
>
> On 10/12/2021 11:34, Jason Andryuk wrote:
> > commit f37f29d31488 "xen: slightly simplify bufioreq handling" hard
> > coded setting req.count = 1 during initial field setup before the main
> > loop.  This missed a subtlety that an early exit from the loop when
> > there are no ioreqs to process, would have req.count == 0 for the return
> > value.  handle_buffered_io() would then remove state->buffered_io_timer.
> > Instead handle_buffered_iopage() is basically always returning true and
> > handle_buffered_io() always re-setting the timer.
> >
> > Restore the disabling of the timer by introducing a new handled_ioreq
> > boolean and use as the return value.  The named variable will more
> > clearly show the intent of the code.
> >
> > Signed-off-by: Jason Andryuk 
>
> Reviewed-by: Paul Durrant 

Thanks, Paul.

What is the next step for getting this into QEMU?

To re-state more plainly, this patch fixes a bug to let QEMU go idle
for longer stretches of time.  Without it, buffer_io_timer continues
to re-arm and fire every 100ms even if there is nothing to do.

Regards,
Jason

[PATCH] xen-hvm: Allow disabling buffer_io_timer

[Jason Andryuk]

commit f37f29d31488 "xen: slightly simplify bufioreq handling" hard
coded setting req.count = 1 during initial field setup before the main
loop.  This missed a subtlety that an early exit from the loop when
there are no ioreqs to process, would have req.count == 0 for the return
value.  handle_buffered_io() would then remove state->buffered_io_timer.
Instead handle_buffered_iopage() is basically always returning true and
handle_buffered_io() always re-setting the timer.

Restore the disabling of the timer by introducing a new handled_ioreq
boolean and use as the return value.  The named variable will more
clearly show the intent of the code.

Signed-off-by: Jason Andryuk 
---
 hw/i386/xen/xen-hvm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 482be95415..cf8e500514 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1087,10 +1087,11 @@ static void handle_ioreq(XenIOState *state, ioreq_t 
*req)
 }
 }
 
-static int handle_buffered_iopage(XenIOState *state)
+static bool handle_buffered_iopage(XenIOState *state)
 {
 buffered_iopage_t *buf_page = state->buffered_io_page;
 buf_ioreq_t *buf_req = NULL;
+bool handled_ioreq = false;
 ioreq_t req;
 int qw;
 
@@ -1144,9 +1145,10 @@ static int handle_buffered_iopage(XenIOState *state)
 assert(!req.data_is_ptr);
 
 qatomic_add(&buf_page->read_pointer, qw + 1);
+handled_ioreq = true;
 }
 
-return req.count;
+return handled_ioreq;
 }
 
 static void handle_buffered_io(void *opaque)
-- 
2.33.1

Re: [PATCH v2] qtest: fix 'expression is always false' build failure in qtest_has_accel()

[Jason Andryuk]

On Wed, Oct 27, 2021 at 11:10 AM Igor Mammedov  wrote:
>
> If KVM is disabled or not present, qtest library build
> may fail with:
>libqtest.c: In function 'qtest_has_accel':
>   comparison of unsigned expression < 0 is always false
>   [-Werror=type-limits]
>  for (i = 0; i < ARRAY_SIZE(targets); i++) {
>
> due to empty 'targets' array.
> Fix it by making sure that CONFIG_KVM_TARGETS isn't empty.
>
> Fixes: e741aff0f43343 ("tests: qtest: add qtest_has_accel() to check if 
> tested binary supports accelerator")
> Reported-by: Jason Andryuk 
> Suggested-by: "Michael S. Tsirkin" 
> Signed-off-by: Igor Mammedov 

Tested-by: Jason Andryuk 

Thanks,
Jason

Re: [PATCH] qtest: fix 'expression is always false' build failure in qtest_has_accel()

[Jason Andryuk]

On Wed, Oct 27, 2021 at 4:34 AM Michael S. Tsirkin  wrote:
>
> On Wed, Oct 27, 2021 at 03:45:42AM -0400, Igor Mammedov wrote:
> > If KVM is disabled or not present, qtest library build
> > may fail with:
> >libqtest.c: In function 'qtest_has_accel':
> >   comparison of unsigned expression < 0 is always false
> >   [-Werror=type-limits]
> >  for (i = 0; i < ARRAY_SIZE(targets); i++) {
> >
> > due to empty 'targets' array.
> > Fix it by compiling KVM related part only if
> > CONFIG_KVM_TARGETS is set.
> >
> > Fixes: e741aff0f43343 ("tests: qtest: add qtest_has_accel() to check if 
> > tested binary supports accelerator")
> > Reported-by: Jason Andryuk 
> > Signed-off-by: Igor Mammedov 
>
>
> > ---
> >  tests/qtest/libqtest.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > index 25aeea385b..9833e16f84 100644
> > --- a/tests/qtest/libqtest.c
> > +++ b/tests/qtest/libqtest.c
> > @@ -931,6 +931,7 @@ bool qtest_has_accel(const char *accel_name)
> >  return false;
> >  #endif
> >  } else if (g_str_equal(accel_name, "kvm")) {
> > +#if defined(CONFIG_KVM_TARGETS)
>
> Ugh.
> const char *targets[] = { CONFIG_KVM_TARGETS };
>
> are you use CONFIG_KVM_TARGETS is not defined?
>
> Looks like it's defined, just empty.
>
> which is not standard C BTW.
>
> How about we just add an empty string in meson?
>
> diff --git a/meson.build b/meson.build
> index 2c5b53cbe2..ff85e9c2af 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -75,7 +75,7 @@ else
>kvm_targets = []
>  endif
>
> -kvm_targets_c = ''
> +kvm_targets_c = '""'
>  if not get_option('kvm').disabled() and targetos == 'linux'
>kvm_targets_c = '"' + '" ,"'.join(kvm_targets) + '"'
>  endif

This allows Debian buster (gcc 8.3.0) to compile.

Thanks,
Jason

Re: [PULL v2 02/44] tests: qtest: add qtest_has_accel() to check if tested binary supports accelerator

[Jason Andryuk]

On Wed, Oct 20, 2021 at 6:23 AM Michael S. Tsirkin  wrote:
>
> From: Igor Mammedov 
>
> Currently it is not possible to create tests that have KVM as a hard
> requirement on a host that doesn't support KVM for tested target
> binary (modulo going through the trouble of compiling out
> the offending test case).
>
> Following scenario makes test fail when it's run on non x86 host:
>   qemu-system-x86_64 -enable-kvm -M q35,kernel-irqchip=on -smp 1,maxcpus=288
>
> This patch introduces qtest_has_accel() to let users check if accel is
> available in advance and avoid executing non run-able test-cases.
>
> It implements detection of TCG and KVM only, the rest could be
> added later on, when we actually start testing them in qtest.
>
> Signed-off-by: Igor Mammedov 
> Message-Id: <20210902113551.461632-3-imamm...@redhat.com>
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  tests/qtest/libqos/libqtest.h |  8 
>  tests/qtest/libqtest.c| 27 +++
>  meson.build   |  6 ++
>  3 files changed, 41 insertions(+)
>
> diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> index a68dcd79d4..59e9271195 100644
> --- a/tests/qtest/libqos/libqtest.h
> +++ b/tests/qtest/libqos/libqtest.h
> @@ -588,6 +588,14 @@ bool qtest_big_endian(QTestState *s);
>   */
>  const char *qtest_get_arch(void);
>
> +/**
> + * qtest_has_accel:
> + * @accel_name: Accelerator name to check for.
> + *
> + * Returns: true if the accelerator is built in.
> + */
> +bool qtest_has_accel(const char *accel_name);
> +
>  /**
>   * qtest_add_func:
>   * @str: Test case path.
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 73f6b977a6..25aeea385b 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -922,6 +922,33 @@ const char *qtest_get_arch(void)
>  return end + 1;
>  }
>
> +bool qtest_has_accel(const char *accel_name)
> +{
> +if (g_str_equal(accel_name, "tcg")) {
> +#if defined(CONFIG_TCG)
> +return true;
> +#else
> +return false;
> +#endif
> +} else if (g_str_equal(accel_name, "kvm")) {
> +int i;
> +const char *arch = qtest_get_arch();
> +const char *targets[] = { CONFIG_KVM_TARGETS };
> +
> +for (i = 0; i < ARRAY_SIZE(targets); i++) {

A xen osstest build fails with:
../qemu-xen-dir-remote/tests/qtest/libqtest.c: In function 'qtest_has_accel':
../qemu-xen-dir-remote/tests/qtest/libqtest.c:938:23: error:
comparison of unsigned expression < 0 is always false
[-Werror=type-limits]
 for (i = 0; i < ARRAY_SIZE(targets); i++) {
   ^

Super long osstest log
here:http://logs.test-lab.xenproject.org/osstest/logs/165703/build-i386-xsm/6.ts-xen-build.log

It was configured like:
$source/configure --enable-xen --target-list=i386-softmmu \
--enable-debug \
--enable-trace-backend=log \
--prefix=/usr/local \
--libdir=/usr/local/lib/xen/lib \
--includedir=/usr/local/lib/xen/include \
--extra-cflags="-DXC_WANT_COMPAT_EVTCHN_API=1 \
-DXC_WANT_COMPAT_GNTTAB_API=1 \
-DXC_WANT_COMPAT_MAP_FOREIGN_API=1 \
-DXC_WANT_COMPAT_DEVICEMODEL_API=1 \
" \
--extra-ldflags="-Wl,-rpath,/usr/local/lib/xen/lib" \
--bindir=/usr/local/lib/xen/bin \
--datadir=/usr/local/share/qemu-xen \
--localstatedir=/var \
--docdir=/usr/local/lib/xen/share/doc \
--mandir=/usr/local/lib/xen/share/man \
--libexecdir=/usr/local/lib/xen/libexec \
--firmwarepath=/usr/local/lib/xen/share/qemu-firmware \
--disable-kvm \
--disable-docs \
--disable-guest-agent \
--python=python3 \
--cpu=i386 ;

--cpu=i386 may be important?  osstest is building in a 32bit debian
environment.  My 64bit fedora workstation fails to configure with
--cpu=i386, and it builds successfully without it.

Maybe add #if defined(CONFIG_KVM) around the code like CONFIG_TCG above?

Regards,
Jason

[PATCH] vl: Parse legacy default_machine_opts

[Jason Andryuk]

qemu can't start a xen vm after commit d8fb7d0969d5
"vl: switch -M parsing to keyval" with:

$ ./qemu-system-i386 -M xenfv
Unexpected error in object_property_find_err() at ../qom/object.c:1298:
qemu-system-i386: Property 'xenfv-3.1-machine.accel' not found
Aborted (core dumped)

The default_machine_opts handling doesn't process the legacy machine
options like "accel".  Call qemu_apply_legacy_machine_options to provide
the legacy handling.

Signed-off-by: Jason Andryuk 
---
 softmmu/vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4df1496101..f4d8630fc6 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2126,6 +2126,7 @@ static void qemu_create_machine(QDict *qdict)
 QDict *default_opts =
 keyval_parse(machine_class->default_machine_opts, NULL, NULL,
  &error_abort);
+qemu_apply_legacy_machine_options(default_opts);
 object_set_properties_from_keyval(OBJECT(current_machine), 
default_opts,
   false, &error_abort);
 qobject_unref(default_opts);
-- 
2.30.2

Re: [PULL 36/40] vl: switch -M parsing to keyval

[Jason Andryuk]

On Tue, Jul 6, 2021 at 6:43 AM Paolo Bonzini  wrote:
>
> Switch from QemuOpts to keyval.  This enables the introduction
> of non-scalar machine properties, and JSON syntax in the future.
>
> For JSON syntax to be supported right now, we would have to
> consider what would happen if string-based dictionaries (produced by
> -M key=val) were to be merged with strongly-typed dictionaries
> (produced by -M {'key': 123}).
>
> The simplest way out is to never enter the situation, and only allow one
> -M option when JSON syntax is in use.  However, we want options such as
> -smp to become syntactic sugar for -M, and this is a problem; as soon
> as -smp becomes a shortcut for -M, QEMU would forbid using -M '{}'
> together with -smp.  Therefore, allowing JSON syntax right now for -M
> would be a forward-compatibility nightmare and it would be impossible
> anyway to introduce -M incrementally in tools.
>
> Instead, support for JSON syntax is delayed until after the main
> options are converted to QOM compound properties.  These include -boot,
> -acpitable, -smbios, -m, -semihosting-config, -rtc and -fw_cfg.  Once JSON
> syntax is introduced, these options will _also_ be forbidden together
> with -M '{...}'.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  softmmu/vl.c | 315 ---
>  1 file changed, 146 insertions(+), 169 deletions(-)

Xen's osstest bisector found this commit breaks starting a Xen vm:
https://lore.kernel.org/xen-devel/e1m1hlc-0005ra...@osstest.test-lab.xenproject.org/

qemu fails to start with:
Unexpected error in object_property_find_err() at
../qemu-xen-dir-remote/qom/object.c:1299:
qemu-system-i386: Property 'xenfv-3.1-machine.accel' not found

The Xen machines have `default_machine_opts =
"accel=xen,suppress-vmdesc=on"` which may be the problem?

Regards,
Jason


> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 7dd2d72d0b..f848abd31a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -145,6 +145,8 @@ static const char *cpu_option;
>  static const char *mem_path;
>  static const char *incoming;
>  static const char *loadvm;
> +static const char *accelerators;
> +static QDict *machine_opts_dict;
>  static QTAILQ_HEAD(, ObjectOption) object_opts = 
> QTAILQ_HEAD_INITIALIZER(object_opts);
>  static ram_addr_t maxram_size;
>  static uint64_t ram_slots;
> @@ -235,21 +237,6 @@ static QemuOptsList qemu_option_rom_opts = {
>  },
>  };
>
> -static QemuOptsList qemu_machine_opts = {
> -.name = "machine",
> -.implied_opt_name = "type",
> -.merge_lists = true,
> -.head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
> -.desc = {
> -/*
> - * no elements => accept any
> - * sanity checking will happen later
> - * when setting machine properties
> - */
> -{ }
> -},
> -};
> -
>  static QemuOptsList qemu_accel_opts = {
>  .name = "accel",
>  .implied_opt_name = "accel",
> @@ -498,16 +485,6 @@ static QemuOptsList qemu_action_opts = {
>  },
>  };
>
> -/**
> - * Get machine options
> - *
> - * Returns: machine options (never null).
> - */
> -static QemuOpts *qemu_get_machine_opts(void)
> -{
> -return qemu_find_opts_singleton("machine");
> -}
> -
>  const char *qemu_get_vm_name(void)
>  {
>  return qemu_name;
> @@ -815,33 +792,6 @@ static MachineClass *find_default_machine(GSList 
> *machines)
>  return default_machineclass;
>  }
>
> -static int machine_help_func(QemuOpts *opts, MachineState *machine)
> -{
> -ObjectProperty *prop;
> -ObjectPropertyIterator iter;
> -
> -if (!qemu_opt_has_help_opt(opts)) {
> -return 0;
> -}
> -
> -object_property_iter_init(&iter, OBJECT(machine));
> -while ((prop = object_property_iter_next(&iter))) {
> -if (!prop->set) {
> -continue;
> -}
> -
> -printf("%s.%s=%s", MACHINE_GET_CLASS(machine)->name,
> -   prop->name, prop->type);
> -if (prop->description) {
> -printf(" (%s)\n", prop->description);
> -} else {
> -printf("\n");
> -}
> -}
> -
> -return 1;
> -}
> -
>  static void version(void)
>  {
>  printf("QEMU emulator version " QEMU_FULL_VERSION "\n"
> @@ -1554,33 +1504,31 @@ static gint machine_class_cmp(gconstpointer a, 
> gconstpointer b)
>object_class_get_name(OBJECT_CLASS(mc1)));
>  }
>
> -static MachineClass *machine_parse(const char *name, GSList *machines)
> +static void machine_help_func(const QDict *qdict)
>  {
> -MachineClass *mc;
> -GSList *el;
> +GSList *machines, *el;
> +const char *type = qdict_get_try_str(qdict, "type");
>
> -if (is_help_option(name)) {
> -printf("Supported machines are:\n");
> -machines = g_slist_sort(machines, machine_class_cmp);
> -for (el = machines; el; el = el->next) {
> -MachineClass *mc = el->data;
> -if (mc->alias) {
> -printf("%-20s %s (alias of %s)\n", mc-

Re: [PATCH] meson.build: fix building of Xen support for aarch64

[Jason Andryuk]

On Thu, Oct 29, 2020 at 6:01 AM Alex Bennée  wrote:
>
>
> Stefano Stabellini  writes:
>
> > On Wed, 28 Oct 2020, Alex Bennée wrote:
> >> Xen is supported on aarch64 although weirdly using the i386-softmmu
> >> model. Checking based on the host CPU meant we never enabled Xen
> >> support. It would be nice to enable CONFIG_XEN for aarch64-softmmu to
> >> make it not seem weird but that will require further build surgery.
> >>
> >> Signed-off-by: Alex Bennée 
> >> Cc: Masami Hiramatsu 
> >> Cc: Stefano Stabellini 
> >> Cc: Anthony Perard 
> >> Cc: Paul Durrant 
> >> Fixes: 8a19980e3f ("configure: move accelerator logic to meson")
> >> ---
> >>  meson.build | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index 835424999d..f1fcbfed4c 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -81,6 +81,8 @@ if cpu in ['x86', 'x86_64']
> >>  'CONFIG_HVF': ['x86_64-softmmu'],
> >>  'CONFIG_WHPX': ['i386-softmmu', 'x86_64-softmmu'],
> >>}
> >> +elif cpu in [ 'arm', 'aarch64' ]
> >> +  accelerator_targets += { 'CONFIG_XEN': ['i386-softmmu'] }
> >>  endif
> >
> > This looks very reasonable -- the patch makes sense.

A comment would be useful to explain that Arm needs i386-softmmu for
the xenpv machine.

> >
> > However I have two questions, mostly for my own understanding. I tried
> > to repro the aarch64 build problem but it works at my end, even without
> > this patch.
>
> Building on a x86 host or with cross compiler?
>
> > I wonder why. I suspect it works thanks to these lines in
> > meson.build:

I think it's a runtime and not a build problem.  In osstest, Xen
support was detected and configured, but CONFIG_XEN wasn't set for
Arm.  So at runtime xen_available() returns 0, and QEMU doesn't start
with "qemu-system-i386: -xen-domid 1: Option not supported for this
target"

I posted my investigation here:
https://lore.kernel.org/xen-devel/cakf6xpss8kpgovzrkitpz63bhbvbjxrtywdhekzuo2q1kem...@mail.gmail.com/

Regards,
Jason

Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response

[Jason Andryuk]

On Tue, Oct 27, 2020 at 9:18 AM Mark Cave-Ayland
 wrote:
>
> I spent a bit of time this morning doing some further tests on Linux using 2 
> machines
> and a test program to check CTS and usbmon:
>
> usbmon when adapter unplugged:
> 95a4bf2dd300 2366831506 S Ci:4:004:0 s c0 05   0002 2 <
> 95a4bf2dd300 2366831607 C Ci:4:004:0 0 2 = 0160
>
> usbmon when adapter plugged in and remote connected to minicom:
> 95a4452a79c0 2457273895 S Ci:4:004:0 s c0 05   0002 2 <
> 95a4452a79c0 2457274057 C Ci:4:004:0 0 2 = 3160
>
> It seems I made a mistake here since the response is interpreted as 2 bytes 
> rather
> than a single little-endian word: with CTS asserted on the remote the first 
> byte is
> 0x31 == FTDI_CTS | FTDI_DSR | 1, whilst the 2nd byte is 0x60 == FTDI_THRE | 
> FTDI_TEMT
> which matches the existing QEMU code rather than the comments in ftdi_sio.h.
>
> So sorry for the noise: I'll drop this patch from the series and post a v2 
> shortly.

No worries.  Thanks for investigating.

(I had the thought that maybe reserve bit 0 differentiates one and two
byte responses? Bit 0 clear indicates a 1-byte response and set
indicates a 2 bit response.  But I'm just guessing.)

Regards,
Jason

Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response

[Jason Andryuk]

On Mon, Oct 26, 2020 at 9:40 AM Mark Cave-Ayland
 wrote:
>
> On 26/10/2020 13:00, Jason Andryuk wrote:
>
> > On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  
> > wrote:
> >>
> >> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +, a ecrit:
> >>> On 26/10/2020 09:54, Samuel Thibault wrote:
> >>>> Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +, a ecrit:
> >>>>> The FTDI_GET_MDM_ST response should only return a single byte 
> >>>>> indicating the
> >>>>> modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h 
> >>>>> header
> >>>>> file).
> >>>>>
> >>>>> Signed-off-by: Mark Cave-Ayland 
> >>>>> ---
> >>>>>hw/usb/dev-serial.c | 5 ++---
> >>>>>1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> >>>>> index 4c374d0790..fa734bcf54 100644
> >>>>> --- a/hw/usb/dev-serial.c
> >>>>> +++ b/hw/usb/dev-serial.c
> >>>>> @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice 
> >>>>> *dev, USBPacket *p,
> >>>>>/* TODO: TX ON/OFF */
> >>>>>break;
> >>>>>case VendorDeviceRequest | FTDI_GET_MDM_ST:
> >>>>> -data[0] = usb_get_modem_lines(s) | 1;
> >>>>> -data[1] = FTDI_THRE | FTDI_TEMT;
> >>>>> -p->actual_length = 2;
> >>>>> +data[0] = usb_get_modem_lines(s);
> >>>>> +p->actual_length = 1;
> >>>>
> >> [...]
> >>> A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
> >>> response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
> >>> should be 2 bytes, the comment about B0-B3 being zero and the response 
> >>> from
> >>> my Chip-X above suggests that the "| 1" should still be dropped from the
> >>> response.
> >>
> >> Aurelien, you introduced the "| 1" in
> >>
> >> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> >> Author: Aurelien Jarno 
> >> Date:   Wed Aug 13 04:23:17 2008 +
> >>
> >>  usb-serial: add support for modem lines
> >>
> >> [...]
> >> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, 
> >> int request, int value,
> >>   /* TODO: TX ON/OFF */
> >>   break;
> >>   case DeviceInVendor | FTDI_GET_MDM_ST:
> >> -/* TODO: return modem status */
> >> -data[0] = 0;
> >> -ret = 1;
> >> +data[0] = usb_get_modem_lines(s) | 1;
> >> +data[1] = 0;
> >> +ret = 2;
> >>   break;
> >>
> >> do you know exactly what it is for?
> >
> > Hi,
> >
> > I'm not particularly familiar with the FTDI USB serial devices.  I
> > found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.
> >
> > A little searching found this:
> > https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541
> >
> > That shows "B0   Reserved - must be 1", so maybe that is why "| 1" was 
> > added?
>
> Right - that's for the modem status returned as part of the first 2 status 
> bytes for
> incoming data which is slightly different from modem status returned directly 
> from
> FTDI_SIO_GET_MODEM_STATUS:
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L423.

Okay, sorry for the confusion there.

I thought your "it's the SIO chipsets that return 1 byte which are older
than the chipset being emulated by QEMU" meant you thought your change
to 1 byte was unnecessary.  You also posted two bytes (0x1 0x60) from
your hardware.

> It is the latter which this patch changes and appears to match what I see on 
> my
> Chipi-X hardware here.

I don't have my hardware readily available, but I must have been
seeing 2 bytes from FTDI_GET_MDM_ST with data[1] = FTDI_THRE |
FTDI_TEMT; to make the change.

I don't have the USB captures anymore to compare the lowest bit value.

So right now you are just interested in dropping the lowest bit
setting but preserving the 2 bytes modem status?

Regards,
Jason

Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response

[Jason Andryuk]

On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault  wrote:
>
> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +, a ecrit:
> > On 26/10/2020 09:54, Samuel Thibault wrote:
> > > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +, a ecrit:
> > > > The FTDI_GET_MDM_ST response should only return a single byte 
> > > > indicating the
> > > > modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h 
> > > > header
> > > > file).
> > > >
> > > > Signed-off-by: Mark Cave-Ayland 
> > > > ---
> > > >   hw/usb/dev-serial.c | 5 ++---
> > > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> > > > index 4c374d0790..fa734bcf54 100644
> > > > --- a/hw/usb/dev-serial.c
> > > > +++ b/hw/usb/dev-serial.c
> > > > @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice 
> > > > *dev, USBPacket *p,
> > > >   /* TODO: TX ON/OFF */
> > > >   break;
> > > >   case VendorDeviceRequest | FTDI_GET_MDM_ST:
> > > > -data[0] = usb_get_modem_lines(s) | 1;
> > > > -data[1] = FTDI_THRE | FTDI_TEMT;
> > > > -p->actual_length = 2;
> > > > +data[0] = usb_get_modem_lines(s);
> > > > +p->actual_length = 1;
> > >
> [...]
> > A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
> > response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
> > should be 2 bytes, the comment about B0-B3 being zero and the response from
> > my Chip-X above suggests that the "| 1" should still be dropped from the
> > response.
>
> Aurelien, you introduced the "| 1" in
>
> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> Author: Aurelien Jarno 
> Date:   Wed Aug 13 04:23:17 2008 +
>
> usb-serial: add support for modem lines
>
> [...]
> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int 
> request, int value,
>  /* TODO: TX ON/OFF */
>  break;
>  case DeviceInVendor | FTDI_GET_MDM_ST:
> -/* TODO: return modem status */
> -data[0] = 0;
> -ret = 1;
> +data[0] = usb_get_modem_lines(s) | 1;
> +data[1] = 0;
> +ret = 2;
>  break;
>
> do you know exactly what it is for?

Hi,

I'm not particularly familiar with the FTDI USB serial devices.  I
found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.

A little searching found this:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541

That shows "B0   Reserved - must be 1", so maybe that is why "| 1" was added?

Regards,
Jason

Re: [PATCH v2 0/3] Add Xen CpusAccel

[Jason Andryuk]

On Tue, Oct 13, 2020 at 1:16 PM Paolo Bonzini  wrote:
>
> On 13/10/20 16:05, Jason Andryuk wrote:
> > Xen was left behind when CpusAccel became mandatory and fails the assert
> > in qemu_init_vcpu().  It relied on the same dummy cpu threads as qtest.
> > Move the qtest cpu functions to a common location and reuse them for
> > Xen.
> >
> > v2:
> >   New patch "accel: Remove _WIN32 ifdef from qtest-cpus.c"
> >   Use accel/dummy-cpus.c for filename
> >   Put prototype in include/sysemu/cpus.h
> >
> > Jason Andryuk (3):
> >   accel: Remove _WIN32 ifdef from qtest-cpus.c
> >   accel: move qtest CpusAccel functions to a common location
> >   accel: Add xen CpusAccel using dummy-cpus
> >
> >  accel/{qtest/qtest-cpus.c => dummy-cpus.c} | 27 --
> >  accel/meson.build  |  8 +++
> >  accel/qtest/meson.build|  1 -
> >  accel/qtest/qtest-cpus.h   | 17 --
> >  accel/qtest/qtest.c|  5 +++-
> >  accel/xen/xen-all.c|  8 +++
> >  include/sysemu/cpus.h  |  3 +++
> >  7 files changed, 27 insertions(+), 42 deletions(-)
> >  rename accel/{qtest/qtest-cpus.c => dummy-cpus.c} (71%)
> >  delete mode 100644 accel/qtest/qtest-cpus.h
> >
>
> Acked-by: Paolo Bonzini 

Thank you, Paolo.  Also Anthony Acked and Claudio Reviewed patch 3.
How can we get this into the tree?

Regards,
Jason

Re: [PATCH] hw/xen: Set suppress-vmdesc for Xen machines

[Jason Andryuk]

On Fri, Oct 16, 2020 at 12:44 PM Anthony PERARD
 wrote:
>
> On Fri, Oct 16, 2020 at 12:01:47PM -0400, Jason Andryuk wrote:
> > On Fri, Oct 16, 2020 at 11:38 AM Anthony PERARD
> >  wrote:
> > >
> > > On Tue, Oct 13, 2020 at 03:05:06PM -0400, Jason Andryuk wrote:
> > > > xen-save-devices-state doesn't currently generate a vmdesc, so restore
> > > > always triggers "Expected vmdescription section, but got 0".  This is
> > > > not a problem when restore comes from a file.  However, when QEMU runs
> > > > in a linux stubdom and comes over a console, EOF is not received.  This
> > > > causes a delay restoring - though it does restore.
> > > >
> > > > Setting suppress-vmdesc skips looking for the vmdesc during restore and
> > > > avoids the wait.
> > >
> > > suppress-vmdesc is only used during restore, right? So starting a guest
> > > without it, saving the guest and restoring the guest with
> > > suppress-vmdesc=on added will work as intended? (I'm checking that 
> > > migration
> > > across update of QEMU will work.)
> >
> > vmdesc is a json description of the migration stream that comes after
> > the QEMU migration stream.  For our purposes,  > stream>.  Normal QEMU savevm will generate it,
> > unless suppress-vmdesc is set.  QEMU restore will read it because:
> > "Try to read in the VMDESC section as well, so that dumping tools that
> > intercept our migration stream have the chance to see it."
> >
> > Xen save does not go through savevm, but instead
> > xen-save-devices-state, which is a subset of the QEMU savevm.  It
> > skips RAM since that is read out through Xen interfaces.  Xen uses
> > xen-load-devices-state to restore device state.  That goes through the
> > common qemu_loadvm_state which tries to read the vmdesc stream.
> >
> > For Xen, yes, suppress-vmdesc only matters for the restore case, and
> > it suppresses the attempt to read the vmdesc.  I think every Xen
> > restore currently has "Expected vmdescription section, but got -1" in
> > the -dm.log since the vmdesc is missing.  I have not tested restoring
> > across this change, but since it just controls reading and discarding
> > the vmdesc stream, I don't think it will break migration across
> > update.
>
> Thanks for the explanation.
>
> Acked-by: Anthony PERARD 
>
> Do you think you could send a patch for libxl as well? Since libxl in
> some cases may use the "pc machine instead of "xenfv". I can send the
> patch otherwise.

I should be able to, yes.

Regards,
Jason

Re: [PATCH] hw/xen: Set suppress-vmdesc for Xen machines

[Jason Andryuk]

On Fri, Oct 16, 2020 at 11:38 AM Anthony PERARD
 wrote:
>
> On Tue, Oct 13, 2020 at 03:05:06PM -0400, Jason Andryuk wrote:
> > xen-save-devices-state doesn't currently generate a vmdesc, so restore
> > always triggers "Expected vmdescription section, but got 0".  This is
> > not a problem when restore comes from a file.  However, when QEMU runs
> > in a linux stubdom and comes over a console, EOF is not received.  This
> > causes a delay restoring - though it does restore.
> >
> > Setting suppress-vmdesc skips looking for the vmdesc during restore and
> > avoids the wait.
>
> suppress-vmdesc is only used during restore, right? So starting a guest
> without it, saving the guest and restoring the guest with
> suppress-vmdesc=on added will work as intended? (I'm checking that migration
> across update of QEMU will work.)

vmdesc is a json description of the migration stream that comes after
the QEMU migration stream.  For our purposes, .  Normal QEMU savevm will generate it,
unless suppress-vmdesc is set.  QEMU restore will read it because:
"Try to read in the VMDESC section as well, so that dumping tools that
intercept our migration stream have the chance to see it."

Xen save does not go through savevm, but instead
xen-save-devices-state, which is a subset of the QEMU savevm.  It
skips RAM since that is read out through Xen interfaces.  Xen uses
xen-load-devices-state to restore device state.  That goes through the
common qemu_loadvm_state which tries to read the vmdesc stream.

For Xen, yes, suppress-vmdesc only matters for the restore case, and
it suppresses the attempt to read the vmdesc.  I think every Xen
restore currently has "Expected vmdescription section, but got -1" in
the -dm.log since the vmdesc is missing.  I have not tested restoring
across this change, but since it just controls reading and discarding
the vmdesc stream, I don't think it will break migration across
update.

Regards,
Jason

[PATCH] hw/xen: Set suppress-vmdesc for Xen machines

[Jason Andryuk]

xen-save-devices-state doesn't currently generate a vmdesc, so restore
always triggers "Expected vmdescription section, but got 0".  This is
not a problem when restore comes from a file.  However, when QEMU runs
in a linux stubdom and comes over a console, EOF is not received.  This
causes a delay restoring - though it does restore.

Setting suppress-vmdesc skips looking for the vmdesc during restore and
avoids the wait.

The other approach would be generate a vmdesc in qemu_save_device_state.
Since COLO shared that function, and the vmdesc is just discarded on
restore, we choose to skip it.

Reported-by: Marek Marczykowski-Górecki 
Signed-off-by: Jason Andryuk 
---
 hw/i386/pc_piix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3c2ae0612b..0cf22a57ad 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -987,7 +987,7 @@ static void xenfv_4_2_machine_options(MachineClass *m)
 pc_i440fx_4_2_machine_options(m);
 m->desc = "Xen Fully-virtualized PC";
 m->max_cpus = HVM_MAX_VCPUS;
-m->default_machine_opts = "accel=xen";
+m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
 }
 
 DEFINE_PC_MACHINE(xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,
@@ -999,7 +999,7 @@ static void xenfv_3_1_machine_options(MachineClass *m)
 m->desc = "Xen Fully-virtualized PC";
 m->alias = "xenfv";
 m->max_cpus = HVM_MAX_VCPUS;
-m->default_machine_opts = "accel=xen";
+m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
 }
 
 DEFINE_PC_MACHINE(xenfv, "xenfv-3.1", pc_xen_hvm_init,
-- 
2.25.1

[PATCH v2 3/3] accel: Add xen CpusAccel using dummy-cpus

[Jason Andryuk]

Xen was broken by commit 1583a3898853 ("cpus: extract out qtest-specific
code to accel/qtest").  Xen relied on qemu_init_vcpu() calling
qemu_dummy_start_vcpu() in the default case, but that was replaced by
g_assert_not_reached().

Add a minimal "CpusAccel" for Xen using the dummy-cpus implementation
used by qtest.

Signed-off-by: Jason Andryuk 
---
 accel/meson.build   | 1 +
 accel/xen/xen-all.c | 8 
 2 files changed, 9 insertions(+)

diff --git a/accel/meson.build b/accel/meson.build
index 9a417396bd..b26cca227a 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -12,3 +12,4 @@ dummy_ss.add(files(
 ))
 
 specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
dummy_ss)
+specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss)
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 60b971d0a8..878a4089d9 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -16,6 +16,7 @@
 #include "hw/xen/xen_pt.h"
 #include "chardev/char.h"
 #include "sysemu/accel.h"
+#include "sysemu/cpus.h"
 #include "sysemu/xen.h"
 #include "sysemu/runstate.h"
 #include "migration/misc.h"
@@ -153,6 +154,10 @@ static void xen_setup_post(MachineState *ms, AccelState 
*accel)
 }
 }
 
+const CpusAccel xen_cpus = {
+.create_vcpu_thread = dummy_start_vcpu_thread,
+};
+
 static int xen_init(MachineState *ms)
 {
 MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -180,6 +185,9 @@ static int xen_init(MachineState *ms)
  * opt out of system RAM being allocated by generic code
  */
 mc->default_ram_id = NULL;
+
+cpus_register_accel(&xen_cpus);
+
 return 0;
 }
 
-- 
2.25.1

[PATCH v2 2/3] accel: move qtest CpusAccel functions to a common location

[Jason Andryuk]

Move and rename accel/qtest/qtest-cpus.c files to accel/dummy-cpus.c so
it can be re-used by Xen.

Signed-off-by: Jason Andryuk 

---
v2:
 - Use accel/dummy-cpus.c
 - Put prototype in include/sysemu/cpus.h
---
 accel/{qtest/qtest-cpus.c => dummy-cpus.c} | 22 --
 accel/meson.build  |  7 +++
 accel/qtest/meson.build|  1 -
 accel/qtest/qtest-cpus.h   | 17 -
 accel/qtest/qtest.c|  5 -
 include/sysemu/cpus.h  |  3 +++
 6 files changed, 18 insertions(+), 37 deletions(-)
 rename accel/{qtest/qtest-cpus.c => dummy-cpus.c} (75%)
 delete mode 100644 accel/qtest/qtest-cpus.h

diff --git a/accel/qtest/qtest-cpus.c b/accel/dummy-cpus.c
similarity index 75%
rename from accel/qtest/qtest-cpus.c
rename to accel/dummy-cpus.c
index db094201c1..10429fdfb2 100644
--- a/accel/qtest/qtest-cpus.c
+++ b/accel/dummy-cpus.c
@@ -1,5 +1,5 @@
 /*
- * QTest accelerator code
+ * Dummy cpu thread code
  *
  * Copyright IBM, Corp. 2011
  *
@@ -13,21 +13,12 @@
 
 #include "qemu/osdep.h"
 #include "qemu/rcu.h"
-#include "qapi/error.h"
-#include "qemu/module.h"
-#include "qemu/option.h"
-#include "qemu/config-file.h"
-#include "sysemu/accel.h"
-#include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
-#include "sysemu/cpu-timers.h"
 #include "qemu/guest-random.h"
 #include "qemu/main-loop.h"
 #include "hw/core/cpu.h"
 
-#include "qtest-cpus.h"
-
-static void *qtest_cpu_thread_fn(void *arg)
+static void *dummy_cpu_thread_fn(void *arg)
 {
 CPUState *cpu = arg;
 sigset_t waitset;
@@ -67,7 +58,7 @@ static void *qtest_cpu_thread_fn(void *arg)
 return NULL;
 }
 
-static void qtest_start_vcpu_thread(CPUState *cpu)
+void dummy_start_vcpu_thread(CPUState *cpu)
 {
 char thread_name[VCPU_THREAD_NAME_SIZE];
 
@@ -76,11 +67,6 @@ static void qtest_start_vcpu_thread(CPUState *cpu)
 qemu_cond_init(cpu->halt_cond);
 snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
  cpu->cpu_index);
-qemu_thread_create(cpu->thread, thread_name, qtest_cpu_thread_fn, cpu,
+qemu_thread_create(cpu->thread, thread_name, dummy_cpu_thread_fn, cpu,
QEMU_THREAD_JOINABLE);
 }
-
-const CpusAccel qtest_cpus = {
-.create_vcpu_thread = qtest_start_vcpu_thread,
-.get_virtual_clock = qtest_get_virtual_clock,
-};
diff --git a/accel/meson.build b/accel/meson.build
index bb00d0fd13..9a417396bd 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -5,3 +5,10 @@ subdir('kvm')
 subdir('tcg')
 subdir('xen')
 subdir('stubs')
+
+dummy_ss = ss.source_set()
+dummy_ss.add(files(
+  'dummy-cpus.c',
+))
+
+specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
dummy_ss)
diff --git a/accel/qtest/meson.build b/accel/qtest/meson.build
index e477cb2ae2..a2f3276459 100644
--- a/accel/qtest/meson.build
+++ b/accel/qtest/meson.build
@@ -1,7 +1,6 @@
 qtest_ss = ss.source_set()
 qtest_ss.add(files(
   'qtest.c',
-  'qtest-cpus.c',
 ))
 
 specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
qtest_ss)
diff --git a/accel/qtest/qtest-cpus.h b/accel/qtest/qtest-cpus.h
deleted file mode 100644
index 739519a472..00
--- a/accel/qtest/qtest-cpus.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*
- * Accelerator CPUS Interface
- *
- * Copyright 2020 SUSE LLC
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#ifndef QTEST_CPUS_H
-#define QTEST_CPUS_H
-
-#include "sysemu/cpus.h"
-
-extern const CpusAccel qtest_cpus;
-
-#endif /* QTEST_CPUS_H */
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index 537e8b449c..b282cea5cf 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -25,7 +25,10 @@
 #include "qemu/main-loop.h"
 #include "hw/core/cpu.h"
 
-#include "qtest-cpus.h"
+const CpusAccel qtest_cpus = {
+.create_vcpu_thread = dummy_start_vcpu_thread,
+.get_virtual_clock = qtest_get_virtual_clock,
+};
 
 static int qtest_init_accel(MachineState *ms)
 {
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 231685955d..e8156728c6 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -25,6 +25,9 @@ typedef struct CpusAccel {
 /* register accel-specific cpus interface implementation */
 void cpus_register_accel(const CpusAccel *i);
 
+/* Create a dummy vcpu for CpusAccel->create_vcpu_thread */
+void dummy_start_vcpu_thread(CPUState *);
+
 /* interface available for cpus accelerator threads */
 
 /* For temporary buffers for forming a name */
-- 
2.25.1

[PATCH v2 1/3] accel: Remove _WIN32 ifdef from qtest-cpus.c

[Jason Andryuk]

dummy-cpus.c is only compiled with CONFIG_POSIX, so the _WIN32 condition
will never evaluate true.  Remove it.

Signed-off-by: Jason Andryuk 
---
v2: New in v2
---
 accel/qtest/qtest-cpus.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/accel/qtest/qtest-cpus.c b/accel/qtest/qtest-cpus.c
index 7c5399ed9d..db094201c1 100644
--- a/accel/qtest/qtest-cpus.c
+++ b/accel/qtest/qtest-cpus.c
@@ -29,10 +29,6 @@
 
 static void *qtest_cpu_thread_fn(void *arg)
 {
-#ifdef _WIN32
-error_report("qtest is not supported under Windows");
-exit(1);
-#else
 CPUState *cpu = arg;
 sigset_t waitset;
 int r;
@@ -69,7 +65,6 @@ static void *qtest_cpu_thread_fn(void *arg)
 qemu_mutex_unlock_iothread();
 rcu_unregister_thread();
 return NULL;
-#endif
 }
 
 static void qtest_start_vcpu_thread(CPUState *cpu)
-- 
2.25.1

[PATCH v2 0/3] Add Xen CpusAccel

[Jason Andryuk]

Xen was left behind when CpusAccel became mandatory and fails the assert
in qemu_init_vcpu().  It relied on the same dummy cpu threads as qtest.
Move the qtest cpu functions to a common location and reuse them for
Xen.

v2:
  New patch "accel: Remove _WIN32 ifdef from qtest-cpus.c"
  Use accel/dummy-cpus.c for filename
  Put prototype in include/sysemu/cpus.h

Jason Andryuk (3):
  accel: Remove _WIN32 ifdef from qtest-cpus.c
  accel: move qtest CpusAccel functions to a common location
  accel: Add xen CpusAccel using dummy-cpus

 accel/{qtest/qtest-cpus.c => dummy-cpus.c} | 27 --
 accel/meson.build  |  8 +++
 accel/qtest/meson.build|  1 -
 accel/qtest/qtest-cpus.h   | 17 --
 accel/qtest/qtest.c|  5 +++-
 accel/xen/xen-all.c|  8 +++
 include/sysemu/cpus.h  |  3 +++
 7 files changed, 27 insertions(+), 42 deletions(-)
 rename accel/{qtest/qtest-cpus.c => dummy-cpus.c} (71%)
 delete mode 100644 accel/qtest/qtest-cpus.h

-- 
2.25.1

Re: [PATCH 1/2] accel: move qtest CpusAccel functions to a common location

[Jason Andryuk]

On Mon, Oct 12, 2020 at 4:23 PM Claudio Fontana  wrote:
>
> On 10/12/20 10:07 PM, Jason Andryuk wrote:
> > Move and rename accel/qtest/qtest-cpu.* files to accel/dummy/ so they
> > can be re-used by Xen.
> >
> > Signed-off-by: Jason Andryuk 
> > ---
> >  .../qtest-cpus.c => dummy/dummy-cpus.c}   | 22 +--
> >  .../qtest-cpus.h => dummy/dummy-cpus.h}   | 10 -
> >  accel/dummy/meson.build   |  6 +
> >  accel/meson.build |  1 +
> >  accel/qtest/meson.build   |  1 -
> >  accel/qtest/qtest.c   |  7 +-
> >  6 files changed, 23 insertions(+), 24 deletions(-)
> >  rename accel/{qtest/qtest-cpus.c => dummy/dummy-cpus.c} (76%)
> >  rename accel/{qtest/qtest-cpus.h => dummy/dummy-cpus.h} (59%)
> >  create mode 100644 accel/dummy/meson.build
> >
> > diff --git a/accel/qtest/qtest-cpus.c b/accel/dummy/dummy-cpus.c
> > similarity index 76%
> > rename from accel/qtest/qtest-cpus.c
> > rename to accel/dummy/dummy-cpus.c
> > index 7c5399ed9d..efade99f03 100644
> > --- a/accel/qtest/qtest-cpus.c
> > +++ b/accel/dummy/dummy-cpus.c
> > @@ -1,5 +1,5 @@
> >  /*
> > - * QTest accelerator code
> > + * Dummy cpu thread code
> >   *
> >   * Copyright IBM, Corp. 2011
> >   *
> > @@ -13,21 +13,14 @@
> >
> >  #include "qemu/osdep.h"
> >  #include "qemu/rcu.h"
> > -#include "qapi/error.h"
> > -#include "qemu/module.h"
> > -#include "qemu/option.h"
> > -#include "qemu/config-file.h"
> > -#include "sysemu/accel.h"
> > -#include "sysemu/qtest.h"
> >  #include "sysemu/cpus.h"
> > -#include "sysemu/cpu-timers.h"
> >  #include "qemu/guest-random.h"
> >  #include "qemu/main-loop.h"
> >  #include "hw/core/cpu.h"
> >
> > -#include "qtest-cpus.h"
> > +#include "dummy-cpus.h"
> >
> > -static void *qtest_cpu_thread_fn(void *arg)
> > +static void *dummy_cpu_thread_fn(void *arg)
> >  {
> >  #ifdef _WIN32
> >  error_report("qtest is not supported under Windows");
>
> I wonder if this should be changed to "dummy cpu thread is not supported 
> under Windows".
>
> Does not matter probably.

I left it since I was just moving the file.  But...

> > diff --git a/accel/dummy/meson.build b/accel/dummy/meson.build
> > new file mode 100644
> > index 00..5fbe27de90
> > --- /dev/null
> > +++ b/accel/dummy/meson.build
> > @@ -0,0 +1,6 @@
> > +dummy_ss = ss.source_set()
> > +dummy_ss.add(files(
> > +  'dummy-cpus.c',
> > +))
> > +
> > +specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
> > dummy_ss)

...  I don't really know meson, but this file is guarded by
CONFIG_POSIX?  If that's true, then this ifdef can just go away.

Regards,
Jason

Re: [PATCH 1/2] accel: move qtest CpusAccel functions to a common location

[Jason Andryuk]

On Mon, Oct 12, 2020 at 4:30 PM Paolo Bonzini  wrote:
>
> On 12/10/20 22:23, Claudio Fontana wrote:
> > On 10/12/20 10:07 PM, Jason Andryuk wrote:
> >> Move and rename accel/qtest/qtest-cpu.* files to accel/dummy/ so they
> >> can be re-used by Xen.
> >>
> >> Signed-off-by: Jason Andryuk 
> >> ---
> >>  .../qtest-cpus.c => dummy/dummy-cpus.c}   | 22 +--
> >>  .../qtest-cpus.h => dummy/dummy-cpus.h}   | 10 -
> >>  accel/dummy/meson.build   |  6 +
> >>  accel/meson.build |  1 +
> >>  accel/qtest/meson.build   |  1 -
> >>  accel/qtest/qtest.c   |  7 +-
> >>  6 files changed, 23 insertions(+), 24 deletions(-)
> >>  rename accel/{qtest/qtest-cpus.c => dummy/dummy-cpus.c} (76%)
> >>  rename accel/{qtest/qtest-cpus.h => dummy/dummy-cpus.h} (59%)
> >>  create mode 100644 accel/dummy/meson.build
>
> Maybe accel/dummy-cpus.c, no need to add a new directory.

Sure.

> >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> >> index 537e8b449c..ac54bc8f52 100644
> >> --- a/accel/qtest/qtest.c
> >> +++ b/accel/qtest/qtest.c
> >> @@ -25,7 +25,12 @@
> >>  #include "qemu/main-loop.h"
> >>  #include "hw/core/cpu.h"
> >>
> >> -#include "qtest-cpus.h"
> >> +#include "accel/dummy/dummy-cpus.h"
> >
> > this is a bit strange from my perspective, does not look right.

Yeah, I didn't really know where to put it.

> > Maybe this dummy cpu prototype should be somewhere in include/ ,
> > like include/sysemu/cpus.h or even better include/sysemu/accel.h
>
> Yes, it should be in include/sysemu/cpus.h.

Sounds good.

Thanks,
Jason

[PATCH 2/2] accel: Add xen CpusAccel using dummy-cpu

[Jason Andryuk]

Xen was broken by commit 1583a3898853 ("cpus: extract out qtest-specific
code to accel/qtest").  Xen relied on qemu_init_vcpu() calling
qemu_dummy_start_vcpu() in the default case, but that was replaced by
g_assert_not_reached().

Add a minimal "CpusAccel" for xen using the dummy-cpu implementation
used by qtest.

Signed-off-by: Jason Andryuk 
---
 accel/dummy/meson.build |  1 +
 accel/xen/xen-all.c | 10 ++
 2 files changed, 11 insertions(+)

diff --git a/accel/dummy/meson.build b/accel/dummy/meson.build
index 5fbe27de90..cdff0ba746 100644
--- a/accel/dummy/meson.build
+++ b/accel/dummy/meson.build
@@ -4,3 +4,4 @@ dummy_ss.add(files(
 ))
 
 specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
dummy_ss)
+specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss)
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 60b971d0a8..2d243c58d4 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -16,12 +16,15 @@
 #include "hw/xen/xen_pt.h"
 #include "chardev/char.h"
 #include "sysemu/accel.h"
+#include "sysemu/cpus.h"
 #include "sysemu/xen.h"
 #include "sysemu/runstate.h"
 #include "migration/misc.h"
 #include "migration/global_state.h"
 #include "hw/boards.h"
 
+#include "accel/dummy/dummy-cpus.h"
+
 //#define DEBUG_XEN
 
 #ifdef DEBUG_XEN
@@ -153,6 +156,10 @@ static void xen_setup_post(MachineState *ms, AccelState 
*accel)
 }
 }
 
+const CpusAccel xen_cpus = {
+.create_vcpu_thread = dummy_start_vcpu_thread,
+};
+
 static int xen_init(MachineState *ms)
 {
 MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -180,6 +187,9 @@ static int xen_init(MachineState *ms)
  * opt out of system RAM being allocated by generic code
  */
 mc->default_ram_id = NULL;
+
+cpus_register_accel(&xen_cpus);
+
 return 0;
 }
 
-- 
2.25.1

[PATCH 1/2] accel: move qtest CpusAccel functions to a common location

[Jason Andryuk]

Move and rename accel/qtest/qtest-cpu.* files to accel/dummy/ so they
can be re-used by Xen.

Signed-off-by: Jason Andryuk 
---
 .../qtest-cpus.c => dummy/dummy-cpus.c}   | 22 +--
 .../qtest-cpus.h => dummy/dummy-cpus.h}   | 10 -
 accel/dummy/meson.build   |  6 +
 accel/meson.build |  1 +
 accel/qtest/meson.build   |  1 -
 accel/qtest/qtest.c   |  7 +-
 6 files changed, 23 insertions(+), 24 deletions(-)
 rename accel/{qtest/qtest-cpus.c => dummy/dummy-cpus.c} (76%)
 rename accel/{qtest/qtest-cpus.h => dummy/dummy-cpus.h} (59%)
 create mode 100644 accel/dummy/meson.build

diff --git a/accel/qtest/qtest-cpus.c b/accel/dummy/dummy-cpus.c
similarity index 76%
rename from accel/qtest/qtest-cpus.c
rename to accel/dummy/dummy-cpus.c
index 7c5399ed9d..efade99f03 100644
--- a/accel/qtest/qtest-cpus.c
+++ b/accel/dummy/dummy-cpus.c
@@ -1,5 +1,5 @@
 /*
- * QTest accelerator code
+ * Dummy cpu thread code
  *
  * Copyright IBM, Corp. 2011
  *
@@ -13,21 +13,14 @@
 
 #include "qemu/osdep.h"
 #include "qemu/rcu.h"
-#include "qapi/error.h"
-#include "qemu/module.h"
-#include "qemu/option.h"
-#include "qemu/config-file.h"
-#include "sysemu/accel.h"
-#include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
-#include "sysemu/cpu-timers.h"
 #include "qemu/guest-random.h"
 #include "qemu/main-loop.h"
 #include "hw/core/cpu.h"
 
-#include "qtest-cpus.h"
+#include "dummy-cpus.h"
 
-static void *qtest_cpu_thread_fn(void *arg)
+static void *dummy_cpu_thread_fn(void *arg)
 {
 #ifdef _WIN32
 error_report("qtest is not supported under Windows");
@@ -72,7 +65,7 @@ static void *qtest_cpu_thread_fn(void *arg)
 #endif
 }
 
-static void qtest_start_vcpu_thread(CPUState *cpu)
+void dummy_start_vcpu_thread(CPUState *cpu)
 {
 char thread_name[VCPU_THREAD_NAME_SIZE];
 
@@ -81,11 +74,6 @@ static void qtest_start_vcpu_thread(CPUState *cpu)
 qemu_cond_init(cpu->halt_cond);
 snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
  cpu->cpu_index);
-qemu_thread_create(cpu->thread, thread_name, qtest_cpu_thread_fn, cpu,
+qemu_thread_create(cpu->thread, thread_name, dummy_cpu_thread_fn, cpu,
QEMU_THREAD_JOINABLE);
 }
-
-const CpusAccel qtest_cpus = {
-.create_vcpu_thread = qtest_start_vcpu_thread,
-.get_virtual_clock = qtest_get_virtual_clock,
-};
diff --git a/accel/qtest/qtest-cpus.h b/accel/dummy/dummy-cpus.h
similarity index 59%
rename from accel/qtest/qtest-cpus.h
rename to accel/dummy/dummy-cpus.h
index 739519a472..a7a0469b17 100644
--- a/accel/qtest/qtest-cpus.h
+++ b/accel/dummy/dummy-cpus.h
@@ -7,11 +7,11 @@
  * See the COPYING file in the top-level directory.
  */
 
-#ifndef QTEST_CPUS_H
-#define QTEST_CPUS_H
+#ifndef DUMMY_CPUS_H
+#define DUMMY_CPUS_H
 
-#include "sysemu/cpus.h"
+#include "qemu/typedefs.h"
 
-extern const CpusAccel qtest_cpus;
+void dummy_start_vcpu_thread(CPUState *);
 
-#endif /* QTEST_CPUS_H */
+#endif /* DUMMY_CPUS_H */
diff --git a/accel/dummy/meson.build b/accel/dummy/meson.build
new file mode 100644
index 00..5fbe27de90
--- /dev/null
+++ b/accel/dummy/meson.build
@@ -0,0 +1,6 @@
+dummy_ss = ss.source_set()
+dummy_ss.add(files(
+  'dummy-cpus.c',
+))
+
+specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
dummy_ss)
diff --git a/accel/meson.build b/accel/meson.build
index bb00d0fd13..d45a33fb8e 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -1,5 +1,6 @@
 softmmu_ss.add(files('accel.c'))
 
+subdir('dummy')
 subdir('qtest')
 subdir('kvm')
 subdir('tcg')
diff --git a/accel/qtest/meson.build b/accel/qtest/meson.build
index e477cb2ae2..a2f3276459 100644
--- a/accel/qtest/meson.build
+++ b/accel/qtest/meson.build
@@ -1,7 +1,6 @@
 qtest_ss = ss.source_set()
 qtest_ss.add(files(
   'qtest.c',
-  'qtest-cpus.c',
 ))
 
 specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: 
qtest_ss)
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index 537e8b449c..ac54bc8f52 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -25,7 +25,12 @@
 #include "qemu/main-loop.h"
 #include "hw/core/cpu.h"
 
-#include "qtest-cpus.h"
+#include "accel/dummy/dummy-cpus.h"
+
+const CpusAccel qtest_cpus = {
+.create_vcpu_thread = dummy_start_vcpu_thread,
+.get_virtual_clock = qtest_get_virtual_clock,
+};
 
 static int qtest_init_accel(MachineState *ms)
 {
-- 
2.25.1

[PATCH 0/2] Add Xen CpusAccel

[Jason Andryuk]

Xen was left behind when CpusAccel became mandatory and fails the assert
in qemu_init_vcpu().  It relied on the same dummy cpu threads as qtest.
Move the qtest cpu functions to a common location and reuse them for
Xen.

Jason Andryuk (2):
  accel: move qtest CpusAccel functions to a common location
  accel: Add xen CpusAccel using dummy-cpu

 .../qtest-cpus.c => dummy/dummy-cpus.c}   | 22 +--
 .../qtest-cpus.h => dummy/dummy-cpus.h}   | 10 -
 accel/dummy/meson.build   |  7 ++
 accel/meson.build |  1 +
 accel/qtest/meson.build   |  1 -
 accel/qtest/qtest.c   |  7 +-
 accel/xen/xen-all.c   | 10 +
 7 files changed, 34 insertions(+), 24 deletions(-)
 rename accel/{qtest/qtest-cpus.c => dummy/dummy-cpus.c} (76%)
 rename accel/{qtest/qtest-cpus.h => dummy/dummy-cpus.h} (59%)
 create mode 100644 accel/dummy/meson.build

-- 
2.25.1

[PATCH] vnc-stubs: Allow -vnc none

[Jason Andryuk]

Currently `-vnc none` is fatal when built with `--disable-vnc`.  Make
vnc_parse accept "none", so QEMU still run without using vnc.

Signed-off-by: Jason Andryuk 
---
 ui/vnc-stubs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c
index 06c4ac6296..c6b737dcec 100644
--- a/ui/vnc-stubs.c
+++ b/ui/vnc-stubs.c
@@ -12,6 +12,9 @@ int vnc_display_pw_expire(const char *id, time_t expires)
 };
 QemuOpts *vnc_parse(const char *str, Error **errp)
 {
+if (strcmp(str, "none") == 0) {
+return NULL;
+}
 error_setg(errp, "VNC support is disabled");
 return NULL;
 }
-- 
2.25.1

Re: [PATCH 2/2] xen: cleanup unrealized flash devices

[Jason Andryuk]

On Wed, Jul 1, 2020 at 8:55 AM Philippe Mathieu-Daudé  wrote:
>
> On 7/1/20 2:40 PM, Philippe Mathieu-Daudé wrote:
> > On 7/1/20 2:25 PM, Jason Andryuk wrote:
> >> On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant  wrote:
> >>>
> >>>> -Original Message-
> >>>> From: Philippe Mathieu-Daudé 
> >>>> Sent: 30 June 2020 18:27
> >>>> To: p...@xen.org; xen-de...@lists.xenproject.org; qemu-devel@nongnu.org
> >>>> Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin' 
> >>>> ; 'Paul Durrant'
> >>>> ; 'Jason Andryuk' ; 'Paolo 
> >>>> Bonzini' ;
> >>>> 'Richard Henderson' 
> >>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>>>
> >>>> On 6/30/20 5:44 PM, Paul Durrant wrote:
> >>>>>> -Original Message-
> >>>>>> From: Philippe Mathieu-Daudé 
> >>>>>> Sent: 30 June 2020 16:26
> >>>>>> To: Paul Durrant ; xen-de...@lists.xenproject.org; 
> >>>>>> qemu-devel@nongnu.org
> >>>>>> Cc: Eduardo Habkost ; Michael S. Tsirkin 
> >>>>>> ; Paul Durrant
> >>>>>> ; Jason Andryuk ; Paolo 
> >>>>>> Bonzini ;
> >>>>>> Richard Henderson 
> >>>>>> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >>>>>>
> >>>>>> On 6/24/20 2:18 PM, Paul Durrant wrote:
> >>>>>>> From: Paul Durrant 
> >>>>>>>
> >>>>>>> The generic pc_machine_initfn() calls pc_system_flash_create() which 
> >>>>>>> creates
> >>>>>>> 'system.flash0' and 'system.flash1' devices. These devices are then 
> >>>>>>> realized
> >>>>>>> by pc_system_flash_map() which is called from 
> >>>>>>> pc_system_firmware_init() which
> >>>>>>> itself is called via pc_memory_init(). The latter however is not 
> >>>>>>> called when
> >>>>>>> xen_enable() is true and hence the following assertion fails:
> >>>>>>>
> >>>>>>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> >>>>>>> Assertion `dev->realized' failed
> >>>>>>>
> >>>>>>> These flash devices are unneeded when using Xen so this patch avoids 
> >>>>>>> the
> >>>>>>> assertion by simply removing them using 
> >>>>>>> pc_system_flash_cleanup_unused().
> >>>>>>>
> >>>>>>> Reported-by: Jason Andryuk 
> >>>>>>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with 
> >>>>>>> -blockdev")
> >>>>>>> Signed-off-by: Paul Durrant 
> >>>>>>> Tested-by: Jason Andryuk 
> >>>>>>> ---
> >>>>>>> Cc: Paolo Bonzini 
> >>>>>>> Cc: Richard Henderson 
> >>>>>>> Cc: Eduardo Habkost 
> >>>>>>> Cc: "Michael S. Tsirkin" 
> >>>>>>> Cc: Marcel Apfelbaum 
> >>>>>>> ---
> >>>>>>>  hw/i386/pc_piix.c| 9 ++---
> >>>>>>>  hw/i386/pc_sysfw.c   | 2 +-
> >>>>>>>  include/hw/i386/pc.h | 1 +
> >>>>>>>  3 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>>>> index 1497d0e4ae..977d40afb8 100644
> >>>>>>> --- a/hw/i386/pc_piix.c
> >>>>>>> +++ b/hw/i386/pc_piix.c
> >>>>>>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >>>>>>>  if (!xen_enabled()) {
> >>>>>>>  pc_memory_init(pcms, system_memory,
> >>>>>>> rom_memory, &ram_memory);
> >>>>>>> -} else if (machine->kernel_filename != NULL) {
> >>>>>>> -/* For xen HVM direct kernel boot, load linux here */
> >>>>>>> -xen_load_linux(pcms);
> >>>>>>> +} e

Re: [PATCH 2/2] xen: cleanup unrealized flash devices

[Jason Andryuk]

On Wed, Jul 1, 2020 at 3:03 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Philippe Mathieu-Daudé 
> > Sent: 30 June 2020 18:27
> > To: p...@xen.org; xen-de...@lists.xenproject.org; qemu-devel@nongnu.org
> > Cc: 'Eduardo Habkost' ; 'Michael S. Tsirkin' 
> > ; 'Paul Durrant'
> > ; 'Jason Andryuk' ; 'Paolo 
> > Bonzini' ;
> > 'Richard Henderson' 
> > Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> >
> > On 6/30/20 5:44 PM, Paul Durrant wrote:
> > >> -Original Message-
> > >> From: Philippe Mathieu-Daudé 
> > >> Sent: 30 June 2020 16:26
> > >> To: Paul Durrant ; xen-de...@lists.xenproject.org; 
> > >> qemu-devel@nongnu.org
> > >> Cc: Eduardo Habkost ; Michael S. Tsirkin 
> > >> ; Paul Durrant
> > >> ; Jason Andryuk ; Paolo Bonzini 
> > >> ;
> > >> Richard Henderson 
> > >> Subject: Re: [PATCH 2/2] xen: cleanup unrealized flash devices
> > >>
> > >> On 6/24/20 2:18 PM, Paul Durrant wrote:
> > >>> From: Paul Durrant 
> > >>>
> > >>> The generic pc_machine_initfn() calls pc_system_flash_create() which 
> > >>> creates
> > >>> 'system.flash0' and 'system.flash1' devices. These devices are then 
> > >>> realized
> > >>> by pc_system_flash_map() which is called from pc_system_firmware_init() 
> > >>> which
> > >>> itself is called via pc_memory_init(). The latter however is not called 
> > >>> when
> > >>> xen_enable() is true and hence the following assertion fails:
> > >>>
> > >>> qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > >>> Assertion `dev->realized' failed
> > >>>
> > >>> These flash devices are unneeded when using Xen so this patch avoids the
> > >>> assertion by simply removing them using 
> > >>> pc_system_flash_cleanup_unused().
> > >>>
> > >>> Reported-by: Jason Andryuk 
> > >>> Fixes: ebc29e1beab0 ("pc: Support firmware configuration with 
> > >>> -blockdev")
> > >>> Signed-off-by: Paul Durrant 
> > >>> Tested-by: Jason Andryuk 
> > >>> ---
> > >>> Cc: Paolo Bonzini 
> > >>> Cc: Richard Henderson 
> > >>> Cc: Eduardo Habkost 
> > >>> Cc: "Michael S. Tsirkin" 
> > >>> Cc: Marcel Apfelbaum 
> > >>> ---
> > >>>  hw/i386/pc_piix.c| 9 ++---
> > >>>  hw/i386/pc_sysfw.c   | 2 +-
> > >>>  include/hw/i386/pc.h | 1 +
> > >>>  3 files changed, 8 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > >>> index 1497d0e4ae..977d40afb8 100644
> > >>> --- a/hw/i386/pc_piix.c
> > >>> +++ b/hw/i386/pc_piix.c
> > >>> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> > >>>  if (!xen_enabled()) {
> > >>>  pc_memory_init(pcms, system_memory,
> > >>> rom_memory, &ram_memory);
> > >>> -} else if (machine->kernel_filename != NULL) {
> > >>> -/* For xen HVM direct kernel boot, load linux here */
> > >>> -xen_load_linux(pcms);
> > >>> +} else {
> > >>> +pc_system_flash_cleanup_unused(pcms);
> > >>
> > >> TIL pc_system_flash_cleanup_unused().
> > >>
> > >> What about restricting at the source?
> > >>
> > >
> > > And leave the devices in place? They are not relevant for Xen, so why not 
> > > clean up?
> >
> > No, I meant to not create them in the first place, instead of
> > create+destroy.
> >
> > Anyway what you did works, so I don't have any problem.
>
> IIUC Jason originally tried restricting creation but encountered a problem 
> because xen_enabled() would always return false at that point, because 
> machine creation occurs before accelerators are initialized.

Correct.  Quoting my previous email:
"""
Removing the call to pc_system_flash_create() from pc_machine_initfn()
lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
there since accelerators hav

Re: Migration vmdesc and xen-save-devices-state

[Jason Andryuk]

On Wed, Jun 24, 2020 at 1:57 PM Dr. David Alan Gilbert
 wrote:
>
> * Jason Andryuk (jandr...@gmail.com) wrote:
> > Hi,
> >
> > At some point, QEMU changed to add a json VM description (vmdesc)
> > after the migration data.  The vmdesc is not needed to restore the
> > migration data, but qemu_loadvm_state() will read and discard the
> > vmdesc to clear the stream when should_send_vmdesc() is true.
>
> About 5 years ago :-)

:)

> > xen-save-devices-state generates its migration data without a vmdesc.
> > xen-load-devices-state in turn calls qemu_loadvm_state() which tries
> > to load vmdesc since should_send_vmdesc is true for xen.  When
> > restoring from a file, this is fine since it'll return EOF, print
> > "Expected vmdescription section, but got 0" and end the restore
> > successfully.
> >
> > Linux stubdoms load their migration data over a console, so they don't
> > hit the EOF and end up waiting.  There does seem to be a timeout
> > though and restore continues after a delay, but we'd like to eliminate
> > the delay.
> >
> > Two options to address this are to either:
> > 1) set suppress_vmdesc for the Xen machines to bypass the
> > should_send_vmdesc() check.
> > or
> > 2) just send the vmdesc data.
> >
> > Since vmdesc is just discarded, maybe #1 should be followed.
>
> #1 does sound simple!
>
> > If going with #2, qemu_save_device_state() needs to generate the
> > vmdesc data.  Looking at qemu_save_device_state() and
> > qemu_savevm_state_complete_precopy_non_iterable(), they are both very
> > similar and could seemingly be merged.  qmp_xen_save_devices_state()
> > could even leverage the bdrv_inactivate_all() call in
> > qemu_savevm_state_complete_precopy_non_iterable().
> >
> > The would make qemu_save_device_state a little more heavywight, which
> > could impact COLO.  I'm not sure how performance sensitive the COLO
> > code is, and I haven't measured anything.
>
> COLO snapshots are potentially quite sensitive; although we've got a
> load of other things we could do with speeding up, we could do without
> making them noticably heavier.

qemu_savevm_state_complete_precopy_non_iterable() generates the vmdesc
json and just discards it if not needed.  How much overhead that adds
is the question.

Thanks,
Jason

Migration vmdesc and xen-save-devices-state

[Jason Andryuk]

Hi,

At some point, QEMU changed to add a json VM description (vmdesc)
after the migration data.  The vmdesc is not needed to restore the
migration data, but qemu_loadvm_state() will read and discard the
vmdesc to clear the stream when should_send_vmdesc() is true.

xen-save-devices-state generates its migration data without a vmdesc.
xen-load-devices-state in turn calls qemu_loadvm_state() which tries
to load vmdesc since should_send_vmdesc is true for xen.  When
restoring from a file, this is fine since it'll return EOF, print
"Expected vmdescription section, but got 0" and end the restore
successfully.

Linux stubdoms load their migration data over a console, so they don't
hit the EOF and end up waiting.  There does seem to be a timeout
though and restore continues after a delay, but we'd like to eliminate
the delay.

Two options to address this are to either:
1) set suppress_vmdesc for the Xen machines to bypass the
should_send_vmdesc() check.
or
2) just send the vmdesc data.

Since vmdesc is just discarded, maybe #1 should be followed.

If going with #2, qemu_save_device_state() needs to generate the
vmdesc data.  Looking at qemu_save_device_state() and
qemu_savevm_state_complete_precopy_non_iterable(), they are both very
similar and could seemingly be merged.  qmp_xen_save_devices_state()
could even leverage the bdrv_inactivate_all() call in
qemu_savevm_state_complete_precopy_non_iterable().

The would make qemu_save_device_state a little more heavywight, which
could impact COLO.  I'm not sure how performance sensitive the COLO
code is, and I haven't measured anything.

Does anyone have thoughts or opinions on the subject?

Thanks,
Jason

Re: [PATCH] xen: Fix xen-legacy-backend qdev types

[Jason Andryuk]

On Wed, Jun 24, 2020 at 8:30 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Jason Andryuk 
> > Sent: 24 June 2020 13:20
> > To: Stefano Stabellini ; Anthony Perard 
> > ; Paul
> > Durrant ; xen-de...@lists.xenproject.org
> > Cc: Jason Andryuk ; qemu-devel@nongnu.org
> > Subject: [PATCH] xen: Fix xen-legacy-backend qdev types
> >
> > xen-sysdev is a TYPE_SYS_BUS_DEVICE.  bus_type should not be changed so
> > that it can plug into the System bus.  Otherwise this assert triggers:
> > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> > failed.
> >
> > TYPE_XENBACKEND attaches to TYPE_XENSYSBUS, so its class_init needs to
> > be set accordingly to attach the qdev.  Otherwise the following assert
> > triggers:
> > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> > failed.
> >
> > TYPE_XENBACKEND is not a subclass of XEN_XENSYSDEV, so it's parent
> > is just TYPE_DEVICE.  Change that.
> >
> > Signed-off-by: Jason Andryuk 
>
> Clearly we raced. This patch and my patch #1 are identical so I'm happy to 
> give my ack to this.

Yeah, looks like you beat me by a hair :)

Either way it gets fixed is fine by me.

Thanks,
Jason

[PATCH] xen: Fix xen-legacy-backend qdev types

[Jason Andryuk]

xen-sysdev is a TYPE_SYS_BUS_DEVICE.  bus_type should not be changed so
that it can plug into the System bus.  Otherwise this assert triggers:
qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.

TYPE_XENBACKEND attaches to TYPE_XENSYSBUS, so its class_init needs to
be set accordingly to attach the qdev.  Otherwise the following assert
triggers:
qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.

TYPE_XENBACKEND is not a subclass of XEN_XENSYSDEV, so it's parent
is just TYPE_DEVICE.  Change that.

Signed-off-by: Jason Andryuk 
---
 hw/xen/xen-legacy-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 2335ee2e65..c5c75c0064 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -789,11 +789,12 @@ static void xendev_class_init(ObjectClass *klass, void 
*data)
 set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 /* xen-backend devices can be plugged/unplugged dynamically */
 dc->user_creatable = true;
+dc->bus_type = TYPE_XENSYSBUS;
 }
 
 static const TypeInfo xendev_type_info = {
 .name  = TYPE_XENBACKEND,
-.parent= TYPE_XENSYSDEV,
+.parent= TYPE_DEVICE,
 .class_init= xendev_class_init,
 .instance_size = sizeof(struct XenLegacyDevice),
 };
@@ -824,7 +825,6 @@ static void xen_sysdev_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 device_class_set_props(dc, xen_sysdev_properties);
-dc->bus_type = TYPE_XENSYSBUS;
 }
 
 static const TypeInfo xensysdev_info = {
-- 
2.25.1

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Wed, Jun 24, 2020 at 6:29 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Jason Andryuk 
> > Sent: 24 June 2020 04:24
> > To: Paul Durrant 
> > Cc: Markus Armbruster ; Mark Cave-Ayland 
> > ; Anthony
> > PERARD ; xen-devel 
> > ; QEMU  > de...@nongnu.org>
> > Subject: Re: sysbus failed assert for xen_sysdev
> >
> > On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant  wrote:
> > >
> > > > -Original Message-
> > > > From: Markus Armbruster 
> > > > Sent: 23 June 2020 09:41
> > > > To: Jason Andryuk 
> > > > Cc: Mark Cave-Ayland ; Anthony PERARD 
> > > > ;
> > xen-
> > > > devel ; Paul Durrant ; 
> > > > QEMU 
> > > > Subject: Re: sysbus failed assert for xen_sysdev
> > > >
> > > > Jason Andryuk  writes:
> > > > > Then it gets farther... until
> > > > > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > > > > Assertion `dev->realized' failed.
> > > > >
> > > > > dev->id is NULL. The failing device is:
> > > > > (gdb) p *dev.parent_obj.class.type
> > > > > $12 = {name = 0x56207770 "cfi.pflash01",
> > > > >
> > >
> > > Having commented out the call to xen_be_init() entirely (and 
> > > xen_bus_init() for good measure) I also
> > get this assertion failure, so
> > > I don't think is related.
> >
> > Yes, this is something different.  pc_pflash_create() calls
> > qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> > pc_system_flash_map()...  and pc_system_flash_map() isn't called for
> > Xen.
> >
> > Removing the call to pc_system_flash_create() from pc_machine_initfn()
> > lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> > there since accelerators have not been initialized yes, I guess?
>
> Looks like it can be worked round by the following:

Yes, thank you.

Tested-by: Jason Andryuk 

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1497d0e4ae..977d40afb8 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>  if (!xen_enabled()) {
>  pc_memory_init(pcms, system_memory,
> rom_memory, &ram_memory);
> -} else if (machine->kernel_filename != NULL) {
> -/* For xen HVM direct kernel boot, load linux here */
> -xen_load_linux(pcms);
> +} else {
> +pc_system_flash_cleanup_unused(pcms);
> +if (machine->kernel_filename != NULL) {
> +/* For xen HVM direct kernel boot, load linux here */
> +xen_load_linux(pcms);
> +}
>  }
>
>  gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index ec2a3b3e7e..0ff47a4b59 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
>  }
>  }
>
> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>  {
>  char *prop_name;
>  int i;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e6135c34d6..497f2b7ab7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>
>  /* pc_sysfw.c */
>  void pc_system_flash_create(PCMachineState *pcms);
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>
>  /* acpi-build.c */
>
>
> >
> > Regards,
> > Jason
>

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Tue, Jun 23, 2020 at 9:22 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Jason Andryuk 
> > Sent: 23 June 2020 13:57
> > To: Markus Armbruster 
> > Cc: Mark Cave-Ayland ; Anthony PERARD 
> > ; xen-
> > devel ; Paul Durrant ; QEMU 
> > 
> > Subject: Re: sysbus failed assert for xen_sysdev

> >
> > Thanks for your response, Markus.
> >
> > I didn't write it, but my understanding is as follows.  TYPE_XENSYSDEV
> > is a device on the system bus that provides the TYPE_XENSYSBUS bus.
> > TYPE_XENBACKEND devices can then attach to TYPE_XENSYSBUS.
> >
> > That would make the qom-tree something like:
> >   /TYPE_XENSYSDEV
> > /TYPE_XENSYSBUX
> >   /TYPE_XENBACKEND
> >
> > (I think today the TYPE_XENBACKEND devices ends up attached to the System 
> > bus.)
> >
> > I think TYPE_XENSYSDEV is correct - it is a device on the system bus.
> > static const TypeInfo xensysdev_info = {
> > .name = TYPE_XENSYSDEV,
> > .parent = TYPE_SYS_BUS_DEVICE,
> > ...
> > }
> >
> > TYPE_XENSYSBUS is the xen-specific bus - provided by TYPE_XENSYSDEV -
> > for attaching xendev.
> > static const TypeInfo xensysbus_info = {
> > .name = TYPE_XENSYSBUS,
> > .parent = TYPE_BUS,
> > ...
> > }
> >
> > TYPE_XENBACKEND is a generic Xen device and it plugs into
> > TYPE_XENSYSBUS.  Maybe the .parent here is wrong and it should just be
> > TYPE_DEVICE?
>
> Yes, I think that is the problem leading to the assert. See the equivalent 
> (non-legacy) code in xen-bus.c.

Yes, xen-bus.c looks correct, but the important change seems to be
removing `dc->bus_type = TYPE_XENSYSBUS;` from xen_sysdev_class_init()
and adding it to xendev_class_init().  That let QEMU get to the
cfi.pflash01 assertion failure.

Regards,
Jason

>   Paul
>
> > static const TypeInfo xendev_type_info = {
> > .name = TYPE_XENBACKEND,
> > .parent = TYPE_XENSYSDEV,
> > ...
> > }
> >
> > So removing `bus_type = TYPE_XENSYSBUS` from TYPE_XENSYSDEV class_init
> > and adding it to TYPE_XENBACKEND seems correct to me.

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Markus Armbruster 
> > Sent: 23 June 2020 09:41
> > To: Jason Andryuk 
> > Cc: Mark Cave-Ayland ; Anthony PERARD 
> > ; xen-
> > devel ; Paul Durrant ; QEMU 
> > 
> > Subject: Re: sysbus failed assert for xen_sysdev
> >
> > Jason Andryuk  writes:
> > > Then it gets farther... until
> > > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > > Assertion `dev->realized' failed.
> > >
> > > dev->id is NULL. The failing device is:
> > > (gdb) p *dev.parent_obj.class.type
> > > $12 = {name = 0x56207770 "cfi.pflash01",
> > >
>
> Having commented out the call to xen_be_init() entirely (and xen_bus_init() 
> for good measure) I also get this assertion failure, so
> I don't think is related.

Yes, this is something different.  pc_pflash_create() calls
qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
pc_system_flash_map()...  and pc_system_flash_map() isn't called for
Xen.

Removing the call to pc_system_flash_create() from pc_machine_initfn()
lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
there since accelerators have not been initialized yes, I guess?

Regards,
Jason

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Tue, Jun 23, 2020 at 4:41 AM Markus Armbruster  wrote:
>
> Jason Andryuk  writes:
>
> > On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
> >  wrote:
> >>
> >> On 22/06/2020 21:33, Jason Andryuk wrote:
> >>
> >> > Hi,
> >> >
> >> > Running qemu devel for a Xen VM is failing an assert after the recent
> >> > "qdev: Rework how we plug into the parent bus" sysbus changes.
> >> >
> >> > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> >> > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> >> > failed.
> >> >
> >> > dc->bus_type is "xen-sysbus" and it's the
> >> > `object_dynamic_cast(OBJECT(bus), dc->bus_type)` portion that fails
> >> > the assert.  bus seems to be "main-system-bus", I think:
> >> > (gdb) p *bus
> >> > $3 = {obj = {class = 0x5636d780, free = 0x77c40db0 ,
> >> > properties = 0x563f7180, ref = 3, parent = 0x563fe980}, parent
> >> > = 0x0, name = 0x563fec60 "main-system-bus", ...
> >> >
> >> > The call comes from hw/xen/xen-legacy-backend.c:706
> >> > sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);
> >> >
> >> > Any pointers on what needs to be fixed?
> >>
> >> Hi Jason,
> >>
> >> My understanding is that the assert() is telling you that you're plugging a
> >> TYPE_SYS_BUS_DEVICE into a bus that isn't derived from TYPE_SYSTEM_BUS.
> >> TYPE_SYS_BUS_DEVICE into a bus that isn't derived from TYPE_SYSTEM_BUS. A 
> >> quick look
>
> Correct.  Let's review the assertion:
>
> assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
>
> Context: we're supposted to plug @dev into @bus, and @dc is @dev's
> DeviceClass.
>
> The assertion checks that
>
> 1. @dev plugs into a bus: dc->bus_type
>
> 2. @bus is an instance of the type of bus @dev plugs into:
>object_dynamic_cast(OBJECT(bus), dc->bus_type)
>
> >> at the file in question suggests that you could try changing the parent 
> >> class of
> >> TYPE_XENSYSBUS from TYPE_BUS to TYPE_SYSTEM_BUS to see if that helps?
> >
> > Hi, Mark.
> >
> > Thanks, but unfortunately changing xensysbus_info .parent does not
> > stop the assert.  But it kinda pointed me in the right direction.
> >
> > xen-sysdev overrode the bus_type which was breaking sysbus_realize.
> > So drop that:
> >
> > --- a/hw/xen/xen-legacy-backend.c
> > +++ b/hw/xen/xen-legacy-backend.c
> > @@ -824,7 +825,7 @@ static void xen_sysdev_class_init(ObjectClass
> > *klass, void *data)
> >  DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >  device_class_set_props(dc, xen_sysdev_properties);
> > -dc->bus_type = TYPE_XENSYSBUS;
> > +//dc->bus_type = TYPE_XENSYSBUS;
> >  }
>
> Uff!
>
> Let me explain how things are supposed to work.
>
> Say we have FOO bus (QOM type TYPE_FOO_BUS), with FOO devices plugging
> into it (abstract QOM type TYPE_FOO_DEVICE).  One of them is SOME_FOO
> (concrete QOM type TYPE_SOME_FOO).  Code ties them together like this:
>
> static const TypeInfo pci_bus_info = {
> .name = TYPE_PCI_BUS,
> .parent = TYPE_BUS,
> ...
> };
>
> static const TypeInfo foo_device_info = {
> .name = TYPE_FOO_DEVICE,
> .parent = TYPE_DEVICE,
> .abstract = true,
> .class_init = foo_device_class_init,
> ...
> };
>
> static void foo_device_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> dc->bus_type = TYPE_FOO_BUS;
> ...
> }
>
> static const TypeInfo some_foo_info = {
> .name = TYPE_SOME_FOO,
> .parent = TYPE_FOO_DEVICE,
> ...
> };
>
> When you plug an instance of TYPE_SOME_FOO into a bus, the assertion
> checks that the bus is an instance of TYPE_FOO_BUS.
>
> Note that subtypes of TYPE_FOO_DEVICE do not mess with dc->bus_type!
>
> TYPE_XENSYSDEV does mess with it:
>
> static void xen_sysdev_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> device_class_set_props(dc, xen_sysdev_properties);
> dc->bus_type = TYPE_XENSYSBUS;
> }
>
> static const TypeInfo xensysdev_info = {
> .name 

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
 wrote:
>
> On 22/06/2020 21:33, Jason Andryuk wrote:
>
> > Hi,
> >
> > Running qemu devel for a Xen VM is failing an assert after the recent
> > "qdev: Rework how we plug into the parent bus" sysbus changes.
> >
> > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> > failed.
> >
> > dc->bus_type is "xen-sysbus" and it's the
> > `object_dynamic_cast(OBJECT(bus), dc->bus_type)` portion that fails
> > the assert.  bus seems to be "main-system-bus", I think:
> > (gdb) p *bus
> > $3 = {obj = {class = 0x5636d780, free = 0x77c40db0 ,
> > properties = 0x563f7180, ref = 3, parent = 0x563fe980}, parent
> > = 0x0, name = 0x563fec60 "main-system-bus", ...
> >
> > The call comes from hw/xen/xen-legacy-backend.c:706
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);
> >
> > Any pointers on what needs to be fixed?
>
> Hi Jason,
>
> My understanding is that the assert() is telling you that you're plugging a
> TYPE_SYS_BUS_DEVICE into a bus that isn't derived from TYPE_SYSTEM_BUS. A 
> quick look
> at the file in question suggests that you could try changing the parent class 
> of
> TYPE_XENSYSBUS from TYPE_BUS to TYPE_SYSTEM_BUS to see if that helps?

Hi, Mark.

Thanks, but unfortunately changing xensysbus_info .parent does not
stop the assert.  But it kinda pointed me in the right direction.

xen-sysdev overrode the bus_type which was breaking sysbus_realize.
So drop that:

--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -824,7 +825,7 @@ static void xen_sysdev_class_init(ObjectClass
*klass, void *data)
 DeviceClass *dc = DEVICE_CLASS(klass);

 device_class_set_props(dc, xen_sysdev_properties);
-dc->bus_type = TYPE_XENSYSBUS;
+//dc->bus_type = TYPE_XENSYSBUS;
 }

 static const TypeInfo xensysdev_info = {

Then I had a different instance of the failed assert trying to attach
xen-console-0 to xen-sysbus.  So I made this change:
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -789,6 +789,7 @@ static void xendev_class_init(ObjectClass *klass,
void *data)
 set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 /* xen-backend devices can be plugged/unplugged dynamically */
 dc->user_creatable = true;
+dc->bus_type = TYPE_XENSYSBUS;
 }

 static const TypeInfo xendev_type_info = {

Then it gets farther... until
qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
Assertion `dev->realized' failed.

dev->id is NULL. The failing device is:
(gdb) p *dev.parent_obj.class.type
$12 = {name = 0x56207770 "cfi.pflash01",

Is that right?

I'm going to have to take a break from this now.

Regards,
Jason

sysbus failed assert for xen_sysdev

[Jason Andryuk]

Hi,

Running qemu devel for a Xen VM is failing an assert after the recent
"qdev: Rework how we plug into the parent bus" sysbus changes.

qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.

dc->bus_type is "xen-sysbus" and it's the
`object_dynamic_cast(OBJECT(bus), dc->bus_type)` portion that fails
the assert.  bus seems to be "main-system-bus", I think:
(gdb) p *bus
$3 = {obj = {class = 0x5636d780, free = 0x77c40db0 ,
properties = 0x563f7180, ref = 3, parent = 0x563fe980}, parent
= 0x0, name = 0x563fec60 "main-system-bus", ...

The call comes from hw/xen/xen-legacy-backend.c:706
sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);

Any pointers on what needs to be fixed?

Thanks,
Jason

[PATCH v2 4/4] usb-serial: Fix timeout closing the device

[Jason Andryuk]

Linux guests wait ~30 seconds when closing the emulated /dev/ttyUSB0.
During that time, the kernel driver is sending many control URBs
requesting GetModemStat (5).  Real hardware returns a status with
FTDI_THRE (Transmitter Holding Register) and FTDI_TEMT (Transmitter
Empty) set.  QEMU leaves them clear, and it seems Linux is waiting for
FTDI_TEMT to be set to indicate the tx queue is empty before closing.

Set the bits when responding to a GetModemStat query and avoid the
shutdown delay.

Signed-off-by: Jason Andryuk 
Reviewed-by: Samuel Thibault 

---
Looking at a USB dump for a real FTDI USB adapter, I see these bits set
in all the bulk URBs where QEMU currently has them clear.
---
 hw/usb/dev-serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 77b964588b..d2c03681b7 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -332,7 +332,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 break;
 case DeviceInVendor | FTDI_GET_MDM_ST:
 data[0] = usb_get_modem_lines(s) | 1;
-data[1] = 0;
+data[1] = FTDI_THRE | FTDI_TEMT;
 p->actual_length = 2;
 break;
 case DeviceOutVendor | FTDI_SET_EVENT_CHR:
-- 
2.24.1

[PATCH v2 2/4] usb-serial: chunk data to wMaxPacketSize

[Jason Andryuk]

usb-serial has issues with xHCI controllers where data is lost in the
VM.  Inspecting the URBs in the guest, EHCI starts every 64 byte boundary
(wMaxPacketSize) with a header.  EHCI hands packets into
usb_serial_token_in() with size 64, so these cannot cross the 64 byte
boundary.  The xHCI controller has packets of 512 bytes and the usb-serial
will just write through the 64 byte boundary.  In the guest, this means
data bytes are interpreted as header, so data bytes don't make it out
the serial interface.

Re-work usb_serial_token_in to chunk data into 64 byte units - 2 byte
header and 62 bytes data.  The Linux driver reads wMaxPacketSize to find
the chunk size, so we match that.

Real hardware was observed to pass in 512 byte URBs (496 bytes data +
8 * 2 byte headers).  Since usb-serial only buffers 384 bytes of data,
usb-serial will pass in 6 64 byte blocks and 1 12 byte partial block for
462 bytes max.

Signed-off-by: Jason Andryuk 
---
 hw/usb/dev-serial.c | 47 -
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index d807ce5771..ec091b6a36 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -360,15 +360,16 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 
 static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
 {
-int first_len, len;
+const int max_packet_size = desc_iface0.eps[0].wMaxPacketSize;
+int packet_len;
 uint8_t header[2];
 
-first_len = RECV_BUF - s->recv_ptr;
-len = p->iov.size;
-if (len <= 2) {
+packet_len = p->iov.size;
+if (packet_len <= 2) {
 p->status = USB_RET_NAK;
 return;
 }
+
 header[0] = usb_get_modem_lines(s) | 1;
 /* We do not have the uart details */
 /* handle serial break */
@@ -380,24 +381,34 @@ static void usb_serial_token_in(USBSerialState *s, 
USBPacket *p)
 } else {
 header[1] = 0;
 }
-len -= 2;
-if (len > s->recv_used) {
-len = s->recv_used;
-}
-if (!len) {
+
+if (!s->recv_used) {
 p->status = USB_RET_NAK;
 return;
 }
-if (first_len > len) {
-first_len = len;
-}
-usb_packet_copy(p, header, 2);
-usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
-if (len > first_len) {
-usb_packet_copy(p, s->recv_buf, len - first_len);
+
+while (s->recv_used && packet_len > 2) {
+int first_len, len;
+
+len = MIN(packet_len, max_packet_size);
+len -= 2;
+if (len > s->recv_used) {
+len = s->recv_used;
+}
+
+first_len = RECV_BUF - s->recv_ptr;
+if (first_len > len) {
+first_len = len;
+}
+usb_packet_copy(p, header, 2);
+usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
+if (len > first_len) {
+usb_packet_copy(p, s->recv_buf, len - first_len);
+}
+s->recv_used -= len;
+s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+packet_len -= len + 2;
 }
-s->recv_used -= len;
-s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
 
 return;
 }
-- 
2.24.1

[PATCH v2 1/4] usb-serial: Move USB_TOKEN_IN into a helper function

[Jason Andryuk]

We'll be adding a loop, so move the code into a helper function.  breaks
are replaced with returns.  While making this change, add braces to
single line if statements to comply with coding style and keep
checkpatch happy.

Signed-off-by: Jason Andryuk 
---
v2: Add braces to single line if statements to comply with coding style.

 hw/usb/dev-serial.c | 80 ++---
 1 file changed, 46 insertions(+), 34 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index daac75b7ae..d807ce5771 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -358,13 +358,56 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 }
 }
 
+static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
+{
+int first_len, len;
+uint8_t header[2];
+
+first_len = RECV_BUF - s->recv_ptr;
+len = p->iov.size;
+if (len <= 2) {
+p->status = USB_RET_NAK;
+return;
+}
+header[0] = usb_get_modem_lines(s) | 1;
+/* We do not have the uart details */
+/* handle serial break */
+if (s->event_trigger && s->event_trigger & FTDI_BI) {
+s->event_trigger &= ~FTDI_BI;
+header[1] = FTDI_BI;
+usb_packet_copy(p, header, 2);
+return;
+} else {
+header[1] = 0;
+}
+len -= 2;
+if (len > s->recv_used) {
+len = s->recv_used;
+}
+if (!len) {
+p->status = USB_RET_NAK;
+return;
+}
+if (first_len > len) {
+first_len = len;
+}
+usb_packet_copy(p, header, 2);
+usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
+if (len > first_len) {
+usb_packet_copy(p, s->recv_buf, len - first_len);
+}
+s->recv_used -= len;
+s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+
+return;
+}
+
 static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
 {
 USBSerialState *s = (USBSerialState *)dev;
 uint8_t devep = p->ep->nr;
 struct iovec *iov;
-uint8_t header[2];
-int i, first_len, len;
+int i;
 
 switch (p->pid) {
 case USB_TOKEN_OUT:
@@ -382,38 +425,7 @@ static void usb_serial_handle_data(USBDevice *dev, 
USBPacket *p)
 case USB_TOKEN_IN:
 if (devep != 1)
 goto fail;
-first_len = RECV_BUF - s->recv_ptr;
-len = p->iov.size;
-if (len <= 2) {
-p->status = USB_RET_NAK;
-break;
-}
-header[0] = usb_get_modem_lines(s) | 1;
-/* We do not have the uart details */
-/* handle serial break */
-if (s->event_trigger && s->event_trigger & FTDI_BI) {
-s->event_trigger &= ~FTDI_BI;
-header[1] = FTDI_BI;
-usb_packet_copy(p, header, 2);
-break;
-} else {
-header[1] = 0;
-}
-len -= 2;
-if (len > s->recv_used)
-len = s->recv_used;
-if (!len) {
-p->status = USB_RET_NAK;
-break;
-}
-if (first_len > len)
-first_len = len;
-usb_packet_copy(p, header, 2);
-usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
-if (len > first_len)
-usb_packet_copy(p, s->recv_buf, len - first_len);
-s->recv_used -= len;
-s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+usb_serial_token_in(s, p);
 break;
 
 default:
-- 
2.24.1

[PATCH v2 3/4] usb-serial: Increase receive buffer to 496

[Jason Andryuk]

A FTDI USB adapter on an xHCI controller can send 512 byte USB packets.
These are 8 * ( 2 bytes header + 62 bytes data).  A 384 byte receive
buffer is insufficient to fill a 512 byte packet, so bump the receive
size to 496 ( 512 - 2 * 8 ).

Signed-off-by: Jason Andryuk 
Reviewed-by: Samuel Thibault 
---
 hw/usb/dev-serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index ec091b6a36..77b964588b 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -29,7 +29,7 @@ do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-#define RECV_BUF 384
+#define RECV_BUF (512 - (2 * 8))
 
 /* Commands */
 #define FTDI_RESET 0
-- 
2.24.1

[PATCH v2 0/4] usb-serial: xHCI and timeout fixes

[Jason Andryuk]

This patch series includes two fixes for usb-serial.

The first is a data corruption issue with xHCI controllers.  The FTDI
data packets need to have a 2 byte header start every 64 bytes of packet
data.  For EHCI this is not a problem since USBPacket size is 64, so
only 1 such chunk fits in a packet.  xHCI controllers supply 512 byte
USBPackets, and usb-serial would only write a single header.  This
confuses drivers since they interpret some data bytes as header bytes.
Chunk the data with headers at every 64 byte offset.

To allow full use of the 512 USBPackets, increase the buffer size to 512
- 2 * 8 = 496 bytes.

A second fix is to set the FTDI_THRE (Transmitter Holding Register) and
FTDI_TEMT (Transmitter Empty) status bits in a GetModemStat response.
This makes the linux driver happy when closing the device and avoids a
30 second timeout.

v2: Add braces to single line if statements to comply with coding style.
Added Samuel's R-b to 3 & 4.  1 & 2 only changed by the addition of
braces, but I don't know the protocol for this situation.

Jason Andryuk (4):
  usb-serial: Move USB_TOKEN_IN into a helper function
  usb-serial: chunk data to wMaxPacketSize
  usb-serial: Increase receive buffer to 496
  usb-serial: Fix timeout closing the device

 hw/usb/dev-serial.c | 95 -
 1 file changed, 59 insertions(+), 36 deletions(-)

-- 
2.24.1

Re: [PATCH 1/4] usb-serial: Move USB_TOKEN_IN into a helper function

[Jason Andryuk]

On Mon, Mar 16, 2020 at 7:40 AM Gerd Hoffmann  wrote:
>
>   Hi,
>
> > +if (len > s->recv_used)
> > +len = s->recv_used;
>
> scripts/checkpatch.pl flags a codestyle error here.
>
> > -if (len > s->recv_used)
> > -len = s->recv_used;
>
> Which is strictly speaking not your fault as you are just moving around
> existing code.  It's common practice though that codestyle is fixed up
> too when touching code.  Any chance you can make sure the patches pass
> checkpatch & resend?

Sure.  Will do.

Regards,
Jason

[PATCH 4/4] usb-serial: Fix timeout closing the device

[Jason Andryuk]

Linux guests wait ~30 seconds when closing the emulated /dev/ttyUSB0.
During that time, the kernel driver is sending many control URBs
requesting GetModemStat (5).  Real hardware returns a status with
FTDI_THRE (Transmitter Holding Register) and FTDI_TEMT (Transmitter
Empty) set.  QEMU leaves them clear, and it seems Linux is waiting for
FTDI_TEMT to be set to indicate the tx queue is empty before closing.

Set the bits when responding to a GetModemStat query and avoid the
shutdown delay.

Signed-off-by: Jason Andryuk 

---
Looking at a USB dump for a real FTDI USB adapter, I see these bits set
in all the bulk URBs where QEMU currently has them clear.
---
 hw/usb/dev-serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index ef33bcd127..5389235f17 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -332,7 +332,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 break;
 case DeviceInVendor | FTDI_GET_MDM_ST:
 data[0] = usb_get_modem_lines(s) | 1;
-data[1] = 0;
+data[1] = FTDI_THRE | FTDI_TEMT;
 p->actual_length = 2;
 break;
 case DeviceOutVendor | FTDI_SET_EVENT_CHR:
-- 
2.24.1

[PATCH 0/4] usb-serial: xHCI and timeout fixes

[Jason Andryuk]

This patch series includes two fixes for usb-serial.

The first is a data corruption issue with xHCI controllers.  The FTDI
data packets need to have a 2 byte header start every 64 bytes of packet
data.  For EHCI this is not a problem since USBPacket size is 64, so
only 1 such chunk fits in a packet.  xHCI controllers supply 512 byte
USBPackets, and usb-serial would only write a single header.  This
confuses drivers since they interpret some data bytes as header bytes.
Chunk the data with headers at every 64 byte offset.

To allow full use of the 512 USBPackets, increase the buffer size to 512
- 2 * 8 = 496 bytes.

A second fix is to set the FTDI_THRE (Transmitter Holding Register) and
FTDI_TEMT (Transmitter Empty) status bits in a GetModemStat response.
This makes the linux driver happy when closing the device and avoids a
30 second timeout.

Jason Andryuk (4):
  usb-serial: Move USB_TOKEN_IN into a helper function
  usb-serial: chunk data to wMaxPacketSize
  usb-serial: Increase receive buffer to 496
  usb-serial: Fix timeout closing the device

 hw/usb/dev-serial.c | 92 +++--
 1 file changed, 56 insertions(+), 36 deletions(-)

-- 
2.24.1

[PATCH 3/4] usb-serial: Increase receive buffer to 496

[Jason Andryuk]

A FTDI USB adapter on an xHCI controller can send 512 byte USB packets.
These are 8 * ( 2 bytes header + 62 bytes data).  A 384 byte receive
buffer is insufficient to fill a 512 byte packet, so bump the receive
size to 496 ( 512 - 2 * 8 ).

Signed-off-by: Jason Andryuk 
---
 hw/usb/dev-serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 96b6c34202..ef33bcd127 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -29,7 +29,7 @@ do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-#define RECV_BUF 384
+#define RECV_BUF (512 - (2 * 8))
 
 /* Commands */
 #define FTDI_RESET 0
-- 
2.24.1

[PATCH 1/4] usb-serial: Move USB_TOKEN_IN into a helper function

[Jason Andryuk]

We'll be adding a loop, so move the code into a helper function.  breaks
are replaced with returns.

Signed-off-by: Jason Andryuk 
---
 hw/usb/dev-serial.c | 77 +
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index daac75b7ae..71fa786bd8 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -358,13 +358,53 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 }
 }
 
+static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
+{
+int first_len, len;
+uint8_t header[2];
+
+first_len = RECV_BUF - s->recv_ptr;
+len = p->iov.size;
+if (len <= 2) {
+p->status = USB_RET_NAK;
+return;
+}
+header[0] = usb_get_modem_lines(s) | 1;
+/* We do not have the uart details */
+/* handle serial break */
+if (s->event_trigger && s->event_trigger & FTDI_BI) {
+s->event_trigger &= ~FTDI_BI;
+header[1] = FTDI_BI;
+usb_packet_copy(p, header, 2);
+return;
+} else {
+header[1] = 0;
+}
+len -= 2;
+if (len > s->recv_used)
+len = s->recv_used;
+if (!len) {
+p->status = USB_RET_NAK;
+return;
+}
+if (first_len > len)
+first_len = len;
+usb_packet_copy(p, header, 2);
+usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
+if (len > first_len)
+usb_packet_copy(p, s->recv_buf, len - first_len);
+s->recv_used -= len;
+s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+
+return;
+}
+
 static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
 {
 USBSerialState *s = (USBSerialState *)dev;
 uint8_t devep = p->ep->nr;
 struct iovec *iov;
-uint8_t header[2];
-int i, first_len, len;
+int i;
 
 switch (p->pid) {
 case USB_TOKEN_OUT:
@@ -382,38 +422,7 @@ static void usb_serial_handle_data(USBDevice *dev, 
USBPacket *p)
 case USB_TOKEN_IN:
 if (devep != 1)
 goto fail;
-first_len = RECV_BUF - s->recv_ptr;
-len = p->iov.size;
-if (len <= 2) {
-p->status = USB_RET_NAK;
-break;
-}
-header[0] = usb_get_modem_lines(s) | 1;
-/* We do not have the uart details */
-/* handle serial break */
-if (s->event_trigger && s->event_trigger & FTDI_BI) {
-s->event_trigger &= ~FTDI_BI;
-header[1] = FTDI_BI;
-usb_packet_copy(p, header, 2);
-break;
-} else {
-header[1] = 0;
-}
-len -= 2;
-if (len > s->recv_used)
-len = s->recv_used;
-if (!len) {
-p->status = USB_RET_NAK;
-break;
-}
-if (first_len > len)
-first_len = len;
-usb_packet_copy(p, header, 2);
-usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
-if (len > first_len)
-usb_packet_copy(p, s->recv_buf, len - first_len);
-s->recv_used -= len;
-s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+usb_serial_token_in(s, p);
 break;
 
 default:
-- 
2.24.1

[PATCH 2/4] usb-serial: chunk data to wMaxPacketSize

[Jason Andryuk]

usb-serial has issues with xHCI controllers where data is lost in the
VM.  Inspecting the URBs in the guest, EHCI starts every 64 byte boundary
(wMaxPacketSize) with a header.  EHCI hands packets into
usb_serial_token_in() with size 64, so these cannot cross the 64 byte
boundary.  The xHCI controller has packets of 512 bytes and the usb-serial
will just write through the 64 byte boundary.  In the guest, this means
data bytes are interpreted as header, so data bytes don't make it out
the serial interface.

Re-work usb_serial_token_in to chunk data into 64 byte units - 2 byte
header and 62 bytes data.  The Linux driver reads wMaxPacketSize to find
the chunk size, so we match that.

Real hardware was observed to pass in 512 byte URBs (496 bytes data +
8 * 2 byte headers).  Since usb-serial only buffers 384 bytes of data,
usb-serial will pass in 6 64 byte blocks and 1 12 byte partial block for
462 bytes max.

Signed-off-by: Jason Andryuk 
---
 hw/usb/dev-serial.c | 43 +++
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 71fa786bd8..96b6c34202 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -360,15 +360,16 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 
 static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
 {
-int first_len, len;
+const int max_packet_size = desc_iface0.eps[0].wMaxPacketSize;
+int packet_len;
 uint8_t header[2];
 
-first_len = RECV_BUF - s->recv_ptr;
-len = p->iov.size;
-if (len <= 2) {
+packet_len = p->iov.size;
+if (packet_len <= 2) {
 p->status = USB_RET_NAK;
 return;
 }
+
 header[0] = usb_get_modem_lines(s) | 1;
 /* We do not have the uart details */
 /* handle serial break */
@@ -380,21 +381,31 @@ static void usb_serial_token_in(USBSerialState *s, 
USBPacket *p)
 } else {
 header[1] = 0;
 }
-len -= 2;
-if (len > s->recv_used)
-len = s->recv_used;
-if (!len) {
+
+if (!s->recv_used) {
 p->status = USB_RET_NAK;
 return;
 }
-if (first_len > len)
-first_len = len;
-usb_packet_copy(p, header, 2);
-usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
-if (len > first_len)
-usb_packet_copy(p, s->recv_buf, len - first_len);
-s->recv_used -= len;
-s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+
+while (s->recv_used && packet_len > 2) {
+int first_len, len;
+
+len = MIN(packet_len, max_packet_size);
+len -= 2;
+if (len > s->recv_used)
+len = s->recv_used;
+
+first_len = RECV_BUF - s->recv_ptr;
+if (first_len > len)
+first_len = len;
+usb_packet_copy(p, header, 2);
+usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
+if (len > first_len)
+usb_packet_copy(p, s->recv_buf, len - first_len);
+s->recv_used -= len;
+s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
+packet_len -= len + 2;
+}
 
 return;
 }
-- 
2.24.1

Re: [PATCH] usb-serial: wakeup device on input

[Jason Andryuk]

On Mon, Mar 9, 2020 at 6:08 AM Gerd Hoffmann  wrote:
>
> On Fri, Mar 06, 2020 at 09:09:17AM -0500, Jason Andryuk wrote:
> > Currently usb-serial devices are unable to send data into guests with
> > the xhci controller.  Data is copied into the usb-serial's buffer, but
> > it is not sent into the guest.  Data coming out of the guest works
> > properly.  usb-serial devices work properly with ehci.
> >
> > Have usb-serial call usb_wakeup() when receiving data from the chardev.
> > This seems to notify the xhci controller and fix inbound data flow.
> >
> > Also add USB_CFG_ATT_WAKEUP to the device's bmAttributes.  This matches
> > a real FTDI serial adapter's bmAttributes.
> >
> > Signed-off-by: Jason Andryuk 
>
> Added to usb patch queue.

Thanks.  Unfortunately, while this seemed okay in my early testing,
something is still off.  Typing at slow (human) speed, input seems
fine.  Pasting a large chunk of data into netcat has some of the data
dropped (not coming out of /dev/ttyUSB0 in the guest).  The VM kernel
reports "ttyUSB0: X input overrun(s)" with X seen between 1 and 8.
EHCI seems fine, even for chunks greater than the 384 byte buffer of
usb-serial.

As one example, pasting
Aa0Aa1Aa2Aa3Aa4Aa5Aa6Aa7Aa8Aa9Ab0Ab1Ab2Ab3Ab4Ab5Ab6Ab7Ab8Ab9Ac0Ac1Ac2Ac3Ac4Ac5Ac6Ac7Ac8Ac9Ad0Ad1Ad2Ad3Ad4Ad5Ad6Ad7Ad8Ad9Ae0
looks to only have the following output in the guest
Aa0Aa1Aa2Aa3Aa4Aa5Aa6Aa7Aa8Aa9Ab0Ab1Ab2Ab3Ab4Ab5Ab6Ab7Ab8Ab9Acc1Ac2Ac3Ac4Ac5Ac6Ac7Ac8Ac9Ad0Ad1Ad2Ad3Ad4Ad5Ad6Ad7Ad8Ad9Ae0

Characters 62 & 63 don't make it through "c0Ac" -> "cc".  The guest
kernel does *not* report input overruns in this case.

Any ideas?

Thanks,
Jason

Re: [PATCH] usb-serial: wakeup device on input

[Jason Andryuk]

On Fri, Mar 6, 2020 at 11:13 AM  wrote:
>
> Patchew URL: 
> https://patchew.org/QEMU/20200306140917.26726-1-jandr...@gmail.com/
>
>
>
> Hi,
>
> This series failed the docker-clang@ubuntu build test. Please find the 
> testing commands and
> their output below. If you have Docker installed, you can probably reproduce 
> it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-ubuntu V=1 NETWORK=1
> time make docker-test-clang@ubuntu SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===

I ran these two commands locally and they completed successfully.

>   LINKfp-test
> ---
> dbus-daemon[5453]: Could not get password database information for UID of 
> current process: User "???" unknown or no memory to allocate password entry

Was there a problem with this container's password db and/or
out-of-memory like this message states?

Regards,
Jason

> **
> ERROR:/tmp/qemu-test/src/tests/qtest/dbus-vmstate-test.c:114:get_connection: 
> assertion failed (err == NULL): The connection is closed (g-io-error-quark, 
> 18)
> ERROR - Bail out! 
> ERROR:/tmp/qemu-test/src/tests/qtest/dbus-vmstate-test.c:114:get_connection: 
> assertion failed (err == NULL): The connection is closed (g-io-error-quark, 
> 18)
> Aborted (core dumped)
> cleaning up pid 5453
> make: *** [/tmp/qemu-test/src/tests/Makefile.include:632: check-qtest-i386] 
> Error 1
> make: *** Waiting for unfinished jobs
>
> Looking for expected file 'tests/data/acpi/pc/FACP.bridge'
> ---
> dbus-daemon[6892]: Could not get password database information for UID of 
> current process: User "???" unknown or no memory to allocate password entry
>
> **
> ERROR:/tmp/qemu-test/src/tests/qtest/dbus-vmstate-test.c:114:get_connection: 
> assertion failed (err == NULL): The connection is closed (g-io-error-quark, 
> 18)
> Aborted (core dumped)
> cleaning up pid 6892
> ERROR - Bail out! 
> ERROR:/tmp/qemu-test/src/tests/qtest/dbus-vmstate-test.c:114:get_connection: 
> assertion failed (err == NULL): The connection is closed (g-io-error-quark, 
> 18)
> make: *** [/tmp/qemu-test/src/tests/Makefile.include:632: check-qtest-x86_64] 
> Error 1
>   TESTcheck-qtest-arm: tests/qtest/test-hmp
>   TESTcheck-qtest-arm: tests/qtest/qos-test
>   TESTcheck-qtest-aarch64: tests/qtest/test-hmp
> ---
> raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
> '--label', 'com.qemu.instance.uuid=2e42e92cfb504ed5b5cb56b2c8b512df', '-u', 
> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
> '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', 
> '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
> '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
> '/var/tmp/patchew-tester-tmp-dwbsabkq/src/docker-src.2020-03-06-10.32.46.2194:/var/tmp/qemu:z,ro',
>  'qemu:ubuntu', '/var/tmp/qemu/run', 'test-clang']' returned non-zero exit 
> status 2.
> filter=--filter=label=com.qemu.instance.uuid=2e42e92cfb504ed5b5cb56b2c8b512df
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dwbsabkq/src'
> make: *** [docker-run-test-clang@ubuntu] Error 2
>
> real40m29.624s
> user0m9.675s
>
>
> The full log is available at
> http://patchew.org/logs/20200306140917.26726-1-jandr...@gmail.com/testing.docker-clang@ubuntu/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com

[PATCH] usb-serial: wakeup device on input

[Jason Andryuk]

Currently usb-serial devices are unable to send data into guests with
the xhci controller.  Data is copied into the usb-serial's buffer, but
it is not sent into the guest.  Data coming out of the guest works
properly.  usb-serial devices work properly with ehci.

Have usb-serial call usb_wakeup() when receiving data from the chardev.
This seems to notify the xhci controller and fix inbound data flow.

Also add USB_CFG_ATT_WAKEUP to the device's bmAttributes.  This matches
a real FTDI serial adapter's bmAttributes.

Signed-off-by: Jason Andryuk 

---
Other devices added USB_CFG_ATT_WAKEUP which is why it is repeated here.
Inbound data flow works without it, but it seems like USB_CFG_ATT_WAKEUP
should be set when using usb_wakeup.
---
 hw/usb/dev-serial.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 98465990ef..daac75b7ae 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -98,6 +98,7 @@ do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
 
 typedef struct {
 USBDevice dev;
+USBEndpoint *intr;
 uint8_t recv_buf[RECV_BUF];
 uint16_t recv_ptr;
 uint16_t recv_used;
@@ -153,7 +154,7 @@ static const USBDescDevice desc_device = {
 {
 .bNumInterfaces= 1,
 .bConfigurationValue   = 1,
-.bmAttributes  = USB_CFG_ATT_ONE,
+.bmAttributes  = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
 .bMaxPower = 50,
 .nif = 1,
 .ifs = &desc_iface0,
@@ -459,6 +460,8 @@ static void usb_serial_read(void *opaque, const uint8_t 
*buf, int size)
 memcpy(s->recv_buf + start, buf, size);
 }
 s->recv_used += size;
+
+usb_wakeup(s->intr, 0);
 }
 
 static void usb_serial_event(void *opaque, QEMUChrEvent event)
@@ -513,6 +516,7 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
 if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
 usb_device_attach(dev, &error_abort);
 }
+s->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
 }
 
 static USBDevice *usb_braille_init(USBBus *bus, const char *unused)
-- 
2.24.1

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Tue, Jan 14, 2020 at 5:04 AM Roger Pau Monné  wrote:
>
> On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> > On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk  wrote:
> > >
> > > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné  
> > > wrote:
> > > >
> > > > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > > >  wrote:
> > > > > >
> > > > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > > > >> -Original Message-
> > > > > > >> From: Jason Andryuk [mailto:jandr...@gmail.com]
> > > > > > >> Sent: 14 March 2019 18:16
> > > > > > >> To: Paul Durrant 
> > > > > > >> Cc: qemu-devel@nongnu.org; xen-de...@lists.xenproject.org; 
> > > > > > >> marma...@invisiblethingslab.com; Simon
> > > > > > >> Gaiser ; Stefano Stabellini 
> > > > > > >> ; Anthony Perard
> > > > > > >> 
> > > > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to 
> > > > > > >> XEN_PAGE_SIZE
> > > > > > >>
> > > > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant 
> > > > > > >>  wrote:
> > > > > > >>>> -Original Message-
> > > > > > >>>> From: Jason Andryuk [mailto:jandr...@gmail.com]
> > > > > > >>>> Sent: 11 March 2019 18:02
> > > > > > >>>> To: qemu-devel@nongnu.org
> > > > > > >>>> Cc: xen-de...@lists.xenproject.org; 
> > > > > > >>>> marma...@invisiblethingslab.com; Simon Gaiser
> > > > > > >>>> ; Jason Andryuk 
> > > > > > >>>> ; Stefano Stabellini
> > > > > > >>>> ; Anthony Perard 
> > > > > > >>>> ; Paul Durrant
> > > > > > >>>> 
> > > > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to 
> > > > > > >>>> XEN_PAGE_SIZE
> > > > > > >>>>
> > > > > > >>>> From: Simon Gaiser 
> > > > > > >>>>
> > > > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get 
> > > > > > >>>> located at
> > > > > > >>>> an address which is not page aligned.
> > > > > > >>> IIRC the PCI spec says that the minimum memory region size 
> > > > > > >>> should be at least 4k. Should we even be
> > > > > > >> tolerating BARs smaller than that?
> > > > > > >>>   Paul
> > > > > > >>>
> > > > > > >> Hi, Paul.
> > > > > > >>
> > > > > > >> Simon found this, so it affects a real device.  Simon, do you 
> > > > > > >> recall
> > > > > > >> which device was affected?
> > > > > > >>
> > > > > > >> I think BARs only need to be power-of-two size and aligned, and 
> > > > > > >> 4k is
> > > > > > >> not a minimum.  16bytes may be a minimum, but I don't know what 
> > > > > > >> the
> > > > > > >> spec says.
> > > > > > >>
> > > > > > >> On an Ivy Bridge system, here are some of the devices with BARs 
> > > > > > >> smaller than 4K:
> > > > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > > > >>Memory at d0735000 (64-bit, non-prefetchable) [disabled] 
> > > > > > >> [size=16]
> > > > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series 
> > > > > > >> Chipset
> > > > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 
> > > > > > >> [EHCI])
> > > > > > >>Memory at d0739000 (32-bit, non-prefetchable) [disabled] 
> > > > > > >> [size=1K]
> > > > > > >&

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk  wrote:
>
> On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné  wrote:
> >
> > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > >  wrote:
> > > >
> > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > >> -Original Message-
> > > > >> From: Jason Andryuk [mailto:jandr...@gmail.com]
> > > > >> Sent: 14 March 2019 18:16
> > > > >> To: Paul Durrant 
> > > > >> Cc: qemu-devel@nongnu.org; xen-de...@lists.xenproject.org; 
> > > > >> marma...@invisiblethingslab.com; Simon
> > > > >> Gaiser ; Stefano Stabellini 
> > > > >> ; Anthony Perard
> > > > >> 
> > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to 
> > > > >> XEN_PAGE_SIZE
> > > > >>
> > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant 
> > > > >>  wrote:
> > > > >>>> -Original Message-
> > > > >>>> From: Jason Andryuk [mailto:jandr...@gmail.com]
> > > > >>>> Sent: 11 March 2019 18:02
> > > > >>>> To: qemu-devel@nongnu.org
> > > > >>>> Cc: xen-de...@lists.xenproject.org; 
> > > > >>>> marma...@invisiblethingslab.com; Simon Gaiser
> > > > >>>> ; Jason Andryuk 
> > > > >>>> ; Stefano Stabellini
> > > > >>>> ; Anthony Perard 
> > > > >>>> ; Paul Durrant
> > > > >>>> 
> > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to 
> > > > >>>> XEN_PAGE_SIZE
> > > > >>>>
> > > > >>>> From: Simon Gaiser 
> > > > >>>>
> > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get 
> > > > >>>> located at
> > > > >>>> an address which is not page aligned.
> > > > >>> IIRC the PCI spec says that the minimum memory region size should 
> > > > >>> be at least 4k. Should we even be
> > > > >> tolerating BARs smaller than that?
> > > > >>>   Paul
> > > > >>>
> > > > >> Hi, Paul.
> > > > >>
> > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > >> which device was affected?
> > > > >>
> > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > >> spec says.
> > > > >>
> > > > >> On an Ivy Bridge system, here are some of the devices with BARs 
> > > > >> smaller than 4K:
> > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > >>Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series 
> > > > >> Chipset
> > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > >>Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > >> SMBus Controller (rev 04)
> > > > >>Memory at d0734000 (64-bit, non-prefetchable) [disabled] 
> > > > >> [size=256]
> > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > >> Controller (rev 30)
> > > > >>Memory at d0503000 (32-bit, non-prefetchable) [disabled] 
> > > > >> [size=256]
> > > > >>
> > > > >> These examples are all 4K aligned, so this is not an issue on this 
> > > > >> machine.
> > > > >>
> > > > >> Reviewing the code, I'm now wondering if the following in
> > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:rc =
> > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > >>  XE

Re: [PATCH] qmp: Reset mon->commands on CHR_EVENT_CLOSED

[Jason Andryuk]

On Wed, Nov 13, 2019 at 8:18 AM Marc-André Lureau
 wrote:
>
> Hi
>
> On Wed, Nov 13, 2019 at 4:41 PM Markus Armbruster  wrote:
> >
> > Jason Andryuk  writes:
> >
> > > Currently, mon->commands is uninitialized until CHR_EVENT_OPENED where
> > > it is set to &qmp_cap_negotiation_commands.  After capability
> > > negotiation, it is set to &qmp_commands.  If the chardev is closed,
> > > CHR_EVENT_CLOSED, mon->commands remains as &qmp_commands.  Only once the
> > > chardev is re-opened with CHR_EVENT_OPENED, is it reset to
> > > &qmp_cap_negotiation_commands.
> > >
> > > monitor_qapi_event_emit compares mon->commands to
> > > &qmp_cap_negotiation_commands, and skips sending events when they are
> > > equal.
> >
> > The check's purpose is to ensure we don't send events in capabilities
> > negotiation mode, i.e. between connect and a successful qmp_capabilities
> > command.
> >
> > > In the case of a closed chardev, QMP events are still sent down
> > > to the closed chardev which needs to drop them.
> >
> > True, but I wonder how this can happen.  Can you explain?

I was working on QMP over Xen's argo inter-vm communication for the
OpenXT project.  We had a lock up because we weren't properly dropping
QMP events in the chardev, even though we called CHR_EVENT_CLOSED.  We
fixed the argo chardev to drop messages like other chardevs, but my
investigation found QMP was still sending events even through it was
closed.

Xen's libxl opens a UNIX domain QMP socket for the duration of issuing
a command and then closes it.  OpenXT does the same over Argo.  In the
case of connecting and disconnecting, QMP events continue to be
generated after the first connection, even though there is nothing
connected for an unbounded length of time.  I was just trying to
optimize that scenario by skipping QMP events when disconnected.

> > > Set mon->commands to &qmp_cap_negotiation_commands for CHR_EVENT_CLOSED
> > > to stop sending events.  Setting for the CHR_EVENT_OPENED case remains
> > > since that is how mon->commands is set for a newly opened connections.
> > >
> > > Signed-off-by: Jason Andryuk 
> > > ---
> > >  monitor/qmp.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > > index 9d9e5d8b27..5e2073c5eb 100644
> > > --- a/monitor/qmp.c
> > > +++ b/monitor/qmp.c
> > > @@ -333,6 +333,7 @@ static void monitor_qmp_event(void *opaque, int event)
> >case CHR_EVENT_CLOSED:
> >/*
> > * Note: this is only useful when the output of the chardev
> > * backend is still open.  For example, when the backend is
> > * stdio, it's possible that stdout is still open when stdin
> > >   * is closed.
> > >   */
> > >  monitor_qmp_cleanup_queues(mon);
> > > +mon->commands = &qmp_cap_negotiation_commands;
> > >  json_message_parser_destroy(&mon->parser);
> > >  json_message_parser_init(&mon->parser, handle_qmp_command,
> > >   mon, NULL);
> >
> > Patchew reports this loses SHUTDOWN events.  Local testing confirms it
> > does.  Simplified reproducer:
> >
> > $ for ((i=0; i<100; i++)); do printf '{"execute": 
> > "qmp_capabilities"}\n{"execute": "quit"}' | upstream-qemu -S -display none 
> > -qmp stdio; done | grep -c SHUTDOWN
> > 84
> >
> > In this test, the SHUTDOWN event was lost 16 out of 100 times.
> >
> > Its emission obviously races with the assignment you add.
> >
> > The comment preceding it provides a clue: after CHR_EVENT_CLOSED, we
> > know the input direction is gone, and we duly clean up that part of the
> > monitor.  But the output direction may or may not be gone, so we don't
> > touch it.  Your assignment touches it.  This is not the correct spot.
> > I can't tell you offhand where the correct spot is.  Perhaps Marc-André
> > can.
>
> The same "closed" event is used to signal read-disconnected,
> write-disconnected and hup.
>
> To implement half-closed signaling, we would need more events/state
> (probably change the chardev API to use input and output streams).
> Tbh, I am not sure it's really worth at this point, unless we have a
> "visible" bug to fix.

Yes, I agree.

Thanks for your time looking at this.

Regards,
Jason

Re: [PATCH] qmp: Reset mon->commands on CHR_EVENT_CLOSED

[Jason Andryuk]

On Wed, Nov 6, 2019 at 8:08 PM  wrote:
>
> Patchew URL: 
> https://patchew.org/QEMU/20191106130309.6737-1-jandr...@gmail.com/
>
>
>
> Hi,
>
> This series failed the docker-quick@centos7 build test. Please find the 
> testing commands and
> their output below. If you have Docker installed, you can probably reproduce 
> it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
>
>   TESTiotest-qcow2: 268
> Failures: 060 071 176 184
> Failed 4 of 108 iotests
> make: *** [check-tests/check-block.sh] Error 1
> Traceback (most recent call last):
>   File "./tests/docker/docker.py", line 662, in 
> sys.exit(main())
> ---
> raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
> '--label', 'com.qemu.instance.uuid=cb707bce0c3c456d8ecec70aeb08fddc', '-u', 
> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
> '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', 
> '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
> '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
> '/var/tmp/patchew-tester-tmp-mxl5_jec/src/docker-src.2019-11-06-19.55.47.20736:/var/tmp/qemu:z,ro',
>  'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
> status 2.
> filter=--filter=label=com.qemu.instance.uuid=cb707bce0c3c456d8ecec70aeb08fddc
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-mxl5_jec/src'
> make: *** [docker-run-test-quick@centos7] Error 2
>
> real13m1.810s
> user0m8.371s
>
>
> The full log is available at
> http://patchew.org/logs/20191106130309.6737-1-jandr...@gmail.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com

The full logs shows iotest failures:

Failures: 060 071 176 184
Failed 4 of 108 iotests

The failures are the lack of SHUTDOWN events:
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false, "reason":
"host-qmp-quit"}}

The results are inconsistent though.  On my workstation, for one run I
had test 071 fail, but the others pass.  On another couple of runs,
they all passed.  An a final one, 176 failed.

Looking at 071, they all send a qmp command to exit, but only some of
the tests have an expected shutdown event.  Looking more broadly, it
seems like tests expect shutdown events.

I don't know the code flow, but is it possible on shutdown for the
chardev to be marked closed before the QMP event is generated?  After
this patch, those would not be sent.  If "quit" is expected to always
generate a QMP event, it seems like some ordering needs to be
enforced.

For the tests, the QMP input comes from a shell "here document" ('<<
EOF'), so I suppose stdin could read EOF and mark the chardev closed
before the QMP event is generated.  Before this change, QMP events
would still be generated and stdout would still be connected. Indeed,
chardev/char-fd.c:fd_chr_read() closes the chardev on stdio EOF
regardless of stdout state.

Regards,
Jason

Re: [PATCH] qmp: Reset mon->commands on CHR_EVENT_CLOSED

[Jason Andryuk]

On Wed, Nov 6, 2019 at 9:53 AM Marc-André Lureau
 wrote:
>
> Hi
>
> On Wed, Nov 6, 2019 at 5:04 PM Jason Andryuk  wrote:
> >
> > Currently, mon->commands is uninitialized until CHR_EVENT_OPENED where
> > it is set to &qmp_cap_negotiation_commands.  After capability
> > negotiation, it is set to &qmp_commands.  If the chardev is closed,
> > CHR_EVENT_CLOSED, mon->commands remains as &qmp_commands.  Only once the
> > chardev is re-opened with CHR_EVENT_OPENED, is it reset to
> > &qmp_cap_negotiation_commands.
> >
> > monitor_qapi_event_emit compares mon->commands to
> > &qmp_cap_negotiation_commands, and skips sending events when they are
> > equal.  In the case of a closed chardev, QMP events are still sent down
> > to the closed chardev which needs to drop them.
>
> This is a minor improvement, not really a bug fix or do I read that 
> incorrectly?

Yes, it is more of a minor improvement since disconnected chardevs
already drop the QMP events.  This will just stop generating them in
the first place.

> >
> > Set mon->commands to &qmp_cap_negotiation_commands for CHR_EVENT_CLOSED
> > to stop sending events.  Setting for the CHR_EVENT_OPENED case remains
> > since that is how mon->commands is set for a newly opened connections.
> >
> > Signed-off-by: Jason Andryuk 
> > ---
> >  monitor/qmp.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 9d9e5d8b27..5e2073c5eb 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -333,6 +333,7 @@ static void monitor_qmp_event(void *opaque, int event)
> >   * is closed.
> >   */
> >  monitor_qmp_cleanup_queues(mon);
> > +mon->commands = &qmp_cap_negotiation_commands;
> >  json_message_parser_destroy(&mon->parser);
> >  json_message_parser_init(&mon->parser, handle_qmp_command,
> >   mon, NULL);
> > --
> > 2.21.0
> >
> >
>
> Looks good to me,
> Reviewed-by: Marc-André Lureau 

Thank you.

-Jason

[PATCH] qmp: Reset mon->commands on CHR_EVENT_CLOSED

[Jason Andryuk]

Currently, mon->commands is uninitialized until CHR_EVENT_OPENED where
it is set to &qmp_cap_negotiation_commands.  After capability
negotiation, it is set to &qmp_commands.  If the chardev is closed,
CHR_EVENT_CLOSED, mon->commands remains as &qmp_commands.  Only once the
chardev is re-opened with CHR_EVENT_OPENED, is it reset to
&qmp_cap_negotiation_commands.

monitor_qapi_event_emit compares mon->commands to
&qmp_cap_negotiation_commands, and skips sending events when they are
equal.  In the case of a closed chardev, QMP events are still sent down
to the closed chardev which needs to drop them.

Set mon->commands to &qmp_cap_negotiation_commands for CHR_EVENT_CLOSED
to stop sending events.  Setting for the CHR_EVENT_OPENED case remains
since that is how mon->commands is set for a newly opened connections.

Signed-off-by: Jason Andryuk 
---
 monitor/qmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 9d9e5d8b27..5e2073c5eb 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -333,6 +333,7 @@ static void monitor_qmp_event(void *opaque, int event)
  * is closed.
  */
 monitor_qmp_cleanup_queues(mon);
+mon->commands = &qmp_cap_negotiation_commands;
 json_message_parser_destroy(&mon->parser);
 json_message_parser_init(&mon->parser, handle_qmp_command,
  mon, NULL);
-- 
2.21.0

Re: [Qemu-devel] [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné  wrote:
>
> On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> >  wrote:
> > >
> > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > >> -Original Message-
> > > >> From: Jason Andryuk [mailto:jandr...@gmail.com]
> > > >> Sent: 14 March 2019 18:16
> > > >> To: Paul Durrant 
> > > >> Cc: qemu-devel@nongnu.org; xen-de...@lists.xenproject.org; 
> > > >> marma...@invisiblethingslab.com; Simon
> > > >> Gaiser ; Stefano Stabellini 
> > > >> ; Anthony Perard
> > > >> 
> > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to 
> > > >> XEN_PAGE_SIZE
> > > >>
> > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant 
> > > >>  wrote:
> > > >>>> -Original Message-----
> > > >>>> From: Jason Andryuk [mailto:jandr...@gmail.com]
> > > >>>> Sent: 11 March 2019 18:02
> > > >>>> To: qemu-devel@nongnu.org
> > > >>>> Cc: xen-de...@lists.xenproject.org; marma...@invisiblethingslab.com; 
> > > >>>> Simon Gaiser
> > > >>>> ; Jason Andryuk ; 
> > > >>>> Stefano Stabellini
> > > >>>> ; Anthony Perard 
> > > >>>> ; Paul Durrant
> > > >>>> 
> > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > >>>>
> > > >>>> From: Simon Gaiser 
> > > >>>>
> > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located 
> > > >>>> at
> > > >>>> an address which is not page aligned.
> > > >>> IIRC the PCI spec says that the minimum memory region size should be 
> > > >>> at least 4k. Should we even be
> > > >> tolerating BARs smaller than that?
> > > >>>   Paul
> > > >>>
> > > >> Hi, Paul.
> > > >>
> > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > >> which device was affected?
> > > >>
> > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > >> spec says.
> > > >>
> > > >> On an Ivy Bridge system, here are some of the devices with BARs 
> > > >> smaller than 4K:
> > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > >>Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > >>Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > >> SMBus Controller (rev 04)
> > > >>Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > >> Controller (rev 30)
> > > >>Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > >>
> > > >> These examples are all 4K aligned, so this is not an issue on this 
> > > >> machine.
> > > >>
> > > >> Reviewing the code, I'm now wondering if the following in
> > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:rc =
> > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > >>  XEN_PFN(guest_addr + XC_PAGE_SIZE 
> > > >> - 1),
> > > >>  XEN_PFN(machine_addr + 
> > > >> XC_PAGE_SIZE - 1),
> > > >>  XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > >>  op);
> > > >>
> > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > >> in would be 0xd0501000 which is past the actual location.  Should the
&g

Re: [Qemu-devel] [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
 wrote:
>
> On 15/03/2019 09:17, Paul Durrant wrote:
> >> -Original Message-----
> >> From: Jason Andryuk [mailto:jandr...@gmail.com]
> >> Sent: 14 March 2019 18:16
> >> To: Paul Durrant 
> >> Cc: qemu-devel@nongnu.org; xen-de...@lists.xenproject.org; 
> >> marma...@invisiblethingslab.com; Simon
> >> Gaiser ; Stefano Stabellini 
> >> ; Anthony Perard
> >> 
> >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >>
> >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant  
> >> wrote:
> >>>> -Original Message-
> >>>> From: Jason Andryuk [mailto:jandr...@gmail.com]
> >>>> Sent: 11 March 2019 18:02
> >>>> To: qemu-devel@nongnu.org
> >>>> Cc: xen-de...@lists.xenproject.org; marma...@invisiblethingslab.com; 
> >>>> Simon Gaiser
> >>>> ; Jason Andryuk ; 
> >>>> Stefano Stabellini
> >>>> ; Anthony Perard ; 
> >>>> Paul Durrant
> >>>> 
> >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >>>>
> >>>> From: Simon Gaiser 
> >>>>
> >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> >>>> an address which is not page aligned.
> >>> IIRC the PCI spec says that the minimum memory region size should be at 
> >>> least 4k. Should we even be
> >> tolerating BARs smaller than that?
> >>>   Paul
> >>>
> >> Hi, Paul.
> >>
> >> Simon found this, so it affects a real device.  Simon, do you recall
> >> which device was affected?
> >>
> >> I think BARs only need to be power-of-two size and aligned, and 4k is
> >> not a minimum.  16bytes may be a minimum, but I don't know what the
> >> spec says.
> >>
> >> On an Ivy Bridge system, here are some of the devices with BARs smaller 
> >> than 4K:
> >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> >> Series Chipset Family MEI Controller #1 (rev 04)
> >>Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> >>Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> >> SMBus Controller (rev 04)
> >>Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> >> Controller (rev 30)
> >>Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> >>
> >> These examples are all 4K aligned, so this is not an issue on this machine.
> >>
> >> Reviewing the code, I'm now wondering if the following in
> >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:rc =
> >> xc_domain_memory_mapping(xen_xc, xen_domid,
> >>  XEN_PFN(guest_addr + XC_PAGE_SIZE - 
> >> 1),
> >>  XEN_PFN(machine_addr + XC_PAGE_SIZE - 
> >> 1),
> >>  XEN_PFN(size + XC_PAGE_SIZE - 1),
> >>  op);
> >>
> >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> >> in would be 0xd0501000 which is past the actual location.  Should the
> >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> >>
> >> BARs smaller than a page would also be a problem if BARs for different
> >> devices shared the same page.
> > Exactly. We cannot pass them through with any degree of safety (not that 
> > passthrough of an arbitrary device is a particularly safe thing to do 
> > anyway). The xen-pt code would instead need to trap those BARs and perform 
> > the accesses to the real BAR itself. Ultimately though I think we should be 
> > retiring the xen-pt code in favour of a standalone emulator.
>
> It doesn't matter if the BAR is smaller than 4k, if there are holes next
> to it.
>
> Do we know what the case is in practice for these USB controllers?
>
> If the worst comes to the worst, we can re-enumerate the PCI bus to
> ensure that all bars smaller than 4k still have 4k alignment between
> them.  That

Re: [Qemu-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Jason Andryuk [mailto:jandr...@gmail.com]
> > Sent: 11 March 2019 18:02
> > To: qemu-devel@nongnu.org
> > Cc: xen-de...@lists.xenproject.org; marma...@invisiblethingslab.com; Simon 
> > Gaiser
> > ; Jason Andryuk ; Stefano 
> > Stabellini
> > ; Anthony Perard ; Paul 
> > Durrant
> > 
> > Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >
> > From: Simon Gaiser 
> >
> > If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > an address which is not page aligned.
>
> IIRC the PCI spec says that the minimum memory region size should be at least 
> 4k. Should we even be tolerating BARs smaller than that?
>
>   Paul
>

Hi, Paul.

Simon found this, so it affects a real device.  Simon, do you recall
which device was affected?

I think BARs only need to be power-of-two size and aligned, and 4k is
not a minimum.  16bytes may be a minimum, but I don't know what the
spec says.

On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
00:16.0 Communication controller: Intel Corporation 7 Series/C210
Series Chipset Family MEI Controller #1 (rev 04)
   Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
   Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
SMBus Controller (rev 04)
   Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
Controller (rev 30)
   Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]

These examples are all 4K aligned, so this is not an issue on this machine.

Reviewing the code, I'm now wondering if the following in
hw/xen/xen_pt.c:xen_pt_region_update is wrong:rc =
xc_domain_memory_mapping(xen_xc, xen_domid,
 XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
 XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
 XEN_PFN(size + XC_PAGE_SIZE - 1),
 op);

If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
in would be 0xd0501000 which is past the actual location.  Should the
call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?

BARs smaller than a page would also be a problem if BARs for different
devices shared the same page.

Regards,
Jason

> > This breaks the memory mapping via
> > xc_domain_memory_mapping since this function is page based and the
> > "offset" is therefore lost.
> >
> > Without this patch you will see error like this in the stubdom log:
> >
> >   [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. 
> > @0x0004
> >
> > QubesOS/qubes-issues#2849
> >
> > Signed-off-by: Simon Gaiser 
> > Signed-off-by: Jason Andryuk 
> > ---
> >  hw/xen/xen_pt.c | 10 +++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > index 5539d56c3a..7f680442ee 100644
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -449,9 +449,10 @@ static int 
> > xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> >  /* Register PIO/MMIO BARs */
> >  for (i = 0; i < PCI_ROM_SLOT; i++) {
> >  XenHostPCIIORegion *r = &d->io_regions[i];
> > +pcibus_t r_size = r->size;
> >  uint8_t type;
> >
> > -if (r->base_addr == 0 || r->size == 0) {
> > +if (r->base_addr == 0 || r_size == 0) {
> >  continue;
> >  }
> >
> > @@ -469,15 +470,18 @@ static int 
> > xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> >  type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> >  }
> >  *cmd |= PCI_COMMAND_MEMORY;
> > +
> > +/* Round up to a full page for the hypercall. */
> > +r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
> >  }
> >
> >  memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
> > -  "xen-pci-pt-bar", r->size);
> > +  "xen-pci-pt-bar", r_size);
> >  pci_register_bar(&s->dev, i, type, &s->bar[i]);
> >
> >  XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
> > " base_addr=0x%08"PRIx64" type: %#x)\n",
> > -   i, r->size, r->base_addr, type);
> > +   i, r_size, r->base_addr, type);
> >  }
> >
> >  /* Register expansion ROM address */
> > --
> > 2.20.1
>

Re: [Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

[Jason Andryuk]

On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant  wrote:
>
> > -Original Message-
> > From: Jason Andryuk [mailto:jandr...@gmail.com]
> > Sent: 11 March 2019 18:02
> > To: qemu-devel@nongnu.org
> > Cc: xen-de...@lists.xenproject.org; marma...@invisiblethingslab.com; Jason 
> > Andryuk
> > ; Stefano Stabellini ; Anthony 
> > Perard
> > ; Paul Durrant ; Paolo 
> > Bonzini
> > ; Richard Henderson ; Eduardo 
> > Habkost ;
> > Michael S. Tsirkin ; Marcel Apfelbaum 
> > 
> > Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> >
> > For the xen stubdom case, we'll want xenstore initialized, but we'll
> > want to skip the rest of xen_be_init.  Move the initialization to
> > xen_hvm_init so we can conditionalize calling xen_be_init.
> >
> > xs_domain_open() is deprecated for xs_open(0), so make the replacement
> > as well.
>
> Can you elaborate as to why you need to do this when the code at the top of 
> xen_hvm_init() already opens xenstore for its own purposes, and AFAICT 
> xenstore_update() is only needed if QEMU is implementing a PV backend?
>
>

Hi, Paul.  Thanks for reviewing.

I think you are right, that this basically shouldn't be needed if PV
backends are disabled.  This patch came out of OpenXT, where it is
needed for some out-of-tree patches.  But that doesn't make it
suitable for upstreaming.

However, while reviewing, it looks like the xen accelerator in
hw/xen/xen-common.c:xen_init() registers xen_change_state_handler().
xen_change_state_handler() uses the global xenstore handle and will
exit(1) if NULL.  I'm not sure how to get the XenIOState xenstore
handle over to the accelerator's xen_init.  Outside of that, I think
you are correct that xenstore_update doesn't need to be run when PV
backends are disabled.

Thanks,
Jason

Re: [Qemu-devel] [Xen-devel] [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

On Tue, Mar 12, 2019 at 11:15 AM Jason Andryuk  wrote:

> I'll test to verify whether MSI-X works with
> permissive mode.

Dropping this patch and enabling permissive mode allowed MSI-X to work.

{"execute": "device_add", "arguments": {"driver":
"xen-pci-passthrough", "id": "xen-pci-pt_-03-00.0", "hostaddr":
":00:01.00", "machine_addr": ":03:00.0", "permissive": true}}
[00:07.0] xen_pt_realize: Assigning real physical device 00:01.0 to devfn 0x38
[00:07.0] xen_pt_register_regions: IO region 0 registered
(size=0x2000 base_addr=0xe190 type: 0x4)
[00:07.0] xen_pt_config_reg_init: Offset 0x000e mismatch!
Emulated=0x0080, host=0x, syncing to 0x.
[00:07.0] xen_pt_config_reg_init: Offset 0x0010 mismatch!
Emulated=0x, host=0xe194, syncing to 0xe194.
[00:07.0] xen_pt_config_reg_init: Offset 0x0052 mismatch!
Emulated=0x, host=0x01c3, syncing to 0x0003.
[00:07.0] xen_pt_config_reg_init: Offset 0x0072 mismatch!
Emulated=0x, host=0x0086, syncing to 0x0080.
[00:07.0] xen_pt_config_reg_init: Offset 0x00a4 mismatch!
Emulated=0x, host=0x8fc0, syncing to 0x8fc0.
[00:07.0] xen_pt_config_reg_init: Offset 0x00aa mismatch!
Emulated=0x, host=0x0010, syncing to 0x0010.
[00:07.0] xen_pt_config_reg_init: Offset 0x00b2 mismatch!
Emulated=0x, host=0x1011, syncing to 0x1011.
[00:07.0] xen_pt_msix_init: get MSI-X table BAR base 0xe190
[00:07.0] xen_pt_msix_init: table_off = 0x1000, total_entries = 8
[00:07.0] xen_pt_msix_init: mapping physical MSI-X table to 0x7ad6a82e4000
[00:07.0] xen_pt_config_reg_init: Offset 0x0092 mismatch!
Emulated=0x, host=0x0007, syncing to 0x0007.
[00:07.0] xen_pt_pci_intx: intx=1
[00:07.0] xen_pt_realize: Real physical device 00:01.0 registered successfully
{"return": {}}

hw/xen/xen_pt_msi.c:xen_pt_msix_init() calls open(/dev/mem) and mmaps
it, so I had to add CONFIG_DEVMEM to the stubdom linux .config.
Without /dev/mem:
[00:07.0] xen_pt_msix_init: get MSI-X table BAR base 0xe190
[00:07.0] xen_pt_msix_init: Error: Can't open /dev/mem: No such file
or directory
[00:07.0] xen_pt_msix_size_init: Error: Internal error: Invalid
xen_pt_msix_init.
Failed to initialize 12/15, type = 0x1, rc: -2
[00:07.0] xen_pt_msi_set_enable: disabling MSI.
free(): double free detected in tcache 2

[user@sys-usb ~]$ sudo lspci -v -s 00:07.0
00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
Controller (rev 04) (prog-if 30 [XHCI])
Subsystem: Lenovo Device 21d2
Physical Slot: 7
Flags: bus master, fast devsel, latency 0, IRQ 44
Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
Capabilities: [50] Power Management version 3
Capabilities: [70] MSI: Enable- Count=1/1 Maskable- 64bit+
Capabilities: [90] MSI-X: Enable+ Count=8 Masked-
Capabilities: [a0] Express Endpoint, MSI 00
Kernel driver in use: xhci_hcd
Kernel modules: xhci_pci

Regards,
Jason

Re: [Qemu-devel] [Xen-devel] [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

On Tue, Mar 12, 2019 at 10:13 AM Roger Pau Monné  wrote:
>
> On Tue, Mar 12, 2019 at 09:58:56AM -0400, Jason Andryuk wrote:
> > On Tue, Mar 12, 2019 at 8:38 AM Marek Marczykowski-Górecki
> >  wrote:
> > >
> > > On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> > > > On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > > > > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use 
> > > > > the
> > > > > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> > > >
> > > > I'm afraid this requires some more context. What's the actual issue
> > > > that prevents MSI-X from working?
> > >
> > > At least missing "Fix PCI passthrough for HVM with stubdomain" series,
> > > but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
> > > config space).
> > > Some of it can be worked around by enabling permissive mode. Jason, did
> > > you had a chance to test it with any MSI-X device?
> > > I'm not aware of anything thing particular that breaks MSI-X but not
> > > MSI. Besides much less devices lying around to test MSI-X...
> >
> > OpenXT and Qubes have used a compile time patch that disabled MSI-X
> > for a long time.  The OpenXT patch description doesn't help:
> > """
> > Currently we do not support MSI-X setup for PCI devices passed through.
> >
> > Although the specification mentions that PCI-e devices might implement only
> > MSI-X there is not a lot of those and mostly none that we have encountered 
> > yet.
> > Considering that, we force devices to use MSI by hiding the MSI-X 
> > capability.
> > """
> >
> > To be honest, I didn't question the reasoning and just made the
> > compile-time disabling into a runtime disabling.
> >
> > I tested with a NEC uPD720200 XHCI controller supporting MSI-X.  There
> > was an error related to setting up MSI-X when I failed to pass the
> > "-xen-stubdom" flag.  I can pull that log when I get back to the
> > machine.  With this patch, MSI-X was hidden in the guest, but dom0
> > showed MSI-X present but unused.
>
> Given that Marek is working on a series to make both MSI and MSI-X
> work for pci-passthrough and stubdomains I'm not sure how did you even
> manage to get plain MSI working. Is there something I'm missing?

Marek's series adds a hypercall to enable MSI/MSI-X since they are not
allowed by default in PCI passthrough.  PCI passthrough also has a
permissive mode to allow access to a device's entire PCI configuration
space including enabling MSI.

Qubes 4.0 has an out-of-tree patch that whitelists enabling MSI in
non-permissive mode - I've tested these patches on Qubes 4.0 with the
MSI-X XHCI device where MSI is enabled but not MSI-X.  Other NICs with
only MSI also work.

OpenXT linux stubdoms use permissive mode PCI passthrough, so the
stubdom can program the PCI config space to enable MSI.  I've tested
the patches there, but none of my OpenXT test systems have MSI-X.  MSI
devices work properly.

Marek also tested a "vanilla" linux stubdom version in his osstest
suite, but that doesn't test passthrough.

If Marek's pending patch series or "permissive" mode will enable
MSI/MSI-X, then maybe this patch should just be dropped in favor of
those options.  I'll test to verify whether MSI-X works with
permissive mode.

Regards,
Jason

Re: [Qemu-devel] [Xen-devel] [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

On Tue, Mar 12, 2019 at 8:38 AM Marek Marczykowski-Górecki
 wrote:
>
> On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> > On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> > > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> >
> > I'm afraid this requires some more context. What's the actual issue
> > that prevents MSI-X from working?
>
> At least missing "Fix PCI passthrough for HVM with stubdomain" series,
> but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
> config space).
> Some of it can be worked around by enabling permissive mode. Jason, did
> you had a chance to test it with any MSI-X device?
> I'm not aware of anything thing particular that breaks MSI-X but not
> MSI. Besides much less devices lying around to test MSI-X...

OpenXT and Qubes have used a compile time patch that disabled MSI-X
for a long time.  The OpenXT patch description doesn't help:
"""
Currently we do not support MSI-X setup for PCI devices passed through.

Although the specification mentions that PCI-e devices might implement only
MSI-X there is not a lot of those and mostly none that we have encountered yet.
Considering that, we force devices to use MSI by hiding the MSI-X capability.
"""

To be honest, I didn't question the reasoning and just made the
compile-time disabling into a runtime disabling.

I tested with a NEC uPD720200 XHCI controller supporting MSI-X.  There
was an error related to setting up MSI-X when I failed to pass the
"-xen-stubdom" flag.  I can pull that log when I get back to the
machine.  With this patch, MSI-X was hidden in the guest, but dom0
showed MSI-X present but unused.

Marek, is "Use xc_physdev_msi_set_enable for enabling MSI..." the QEMU
patch you are refer to?  Do you think permissive mode would allow
MSI-X to work without that patch?  I could test that out.

Regards,
Jason

Re: [Qemu-devel] [PATCH 1/6] xen: Introduce -xen-stubdom option

[Jason Andryuk]

On Mon, Mar 11, 2019 at 2:06 PM Paolo Bonzini  wrote:
>
> On 11/03/19 19:02, Jason Andryuk wrote:
> > With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
> > There are a few differences when running in a stubdom compared to dom0.
> > Add the -xen-stubdom option to select this mode at runtime.  The default
> > is off.
>
> This should be "-accel xen,stubdom=on".  You should find examples for
> tcg that explain how to add a suboption to -accel.

Thanks, Paolo.  I'll re-work the option as you suggest.

Regards,
Jason

[Qemu-devel] [PATCH 4/6] xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen

[Jason Andryuk]

When running in a stubdom, we have to inform the hypervisor that the
stubdom and not dom0 is handling the device model.  Explicitly created
ioreq servers are fine, but a call to HVM_PARAM_DM_DOMAIN is needed for
the default ioreq server.

Xen 4.12 removes the default ioreq server.  With that, Xen started
returning an error when setting HVM_PARAM_DM_DOMAIN.  Put the
HVM_PARAM_DM_DOMAIN call in the version compatibility header.  When we
fallback to the default ioreq server, issue the call and don't bother to
check the return value.

Original patch by Anthony PERARD 

Signed-off-by: Jason Andryuk 
---
 include/hw/xen/xen_common.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 9a8155e172..f59f841a43 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -616,6 +616,11 @@ static inline void xen_create_ioreq_server(domid_t dom,
 
 *ioservid = 0;
 use_default_ioreq_server = true;
+
+if (xen_stubdom_enabled()) {
+xc_hvm_param_set(xen_xc, xen_domid, HVM_PARAM_DM_DOMAIN, DOMID_SELF);
+}
+
 trace_xen_default_ioreq_server();
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

[Jason Andryuk]

For the xen stubdom case, we'll want xenstore initialized, but we'll
want to skip the rest of xen_be_init.  Move the initialization to
xen_hvm_init so we can conditionalize calling xen_be_init.

xs_domain_open() is deprecated for xs_open(0), so make the replacement
as well.

Signed-off-by: Jason Andryuk 
---
 hw/i386/xen/xen-hvm.c   | 8 
 hw/xen/xen-legacy-backend.c | 8 
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 2939122e7c..c20c4b27f6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1487,6 +1487,14 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 
 xen_bus_init();
 
+xenstore = xs_open(0);
+if (!xenstore) {
+error_report("Can't connect to xenstored");
+goto err;
+}
+
+qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
+
 /* Initialize backend core & drivers */
 if (xen_be_init() != 0) {
 error_report("xen backend core setup failed");
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 36fd1e9b09..bdf2fa917f 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -683,14 +683,6 @@ int xen_be_init(void)
 {
 xengnttab_handle *gnttabdev;
 
-xenstore = xs_daemon_open();
-if (!xenstore) {
-xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
-return -1;
-}
-
-qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
-
 if (xen_xc == NULL || xen_fmem == NULL) {
 /* Check if xen_init() have been called */
 goto err;
-- 
2.20.1

[Qemu-devel] [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.

A compile-time patch was originally written by James McKenzie


Signed-off-by: Jason Andryuk 
---
 hw/xen/xen_pt_config_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 31ec5add1d..b827a493ea 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -54,6 +54,9 @@ static int xen_pt_hide_dev_cap(const XenHostPCIDevice *d, 
uint8_t grp_id)
 return 1;
 }
 break;
+case PCI_CAP_ID_MSIX:
+/* stubdoms don't support MSI-X so skip it. */
+return xen_stubdom_enabled();
 }
 return 0;
 }
-- 
2.20.1

[Qemu-devel] [PATCH 1/6] xen: Introduce -xen-stubdom option

[Jason Andryuk]

With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
There are a few differences when running in a stubdom compared to dom0.
Add the -xen-stubdom option to select this mode at runtime.  The default
is off.

Signed-off-by: Jason Andryuk 
---
 include/hw/xen/xen.h | 6 ++
 qemu-options.hx  | 7 +++
 vl.c | 8 
 3 files changed, 21 insertions(+)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index ba039c146d..fed3611623 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -21,6 +21,7 @@ enum xen_mode {
 extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
 extern bool xen_domid_restrict;
+extern bool xen_stubdom;
 
 extern bool xen_allowed;
 
@@ -29,6 +30,11 @@ static inline bool xen_enabled(void)
 return xen_allowed;
 }
 
+static inline bool xen_stubdom_enabled(void)
+{
+return xen_stubdom;
+}
+
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
diff --git a/qemu-options.hx b/qemu-options.hx
index 1cf9aac1fe..ba56c3dd9a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3386,6 +3386,10 @@ DEF("xen-domid-restrict", 0, 
QEMU_OPTION_xen_domid_restrict,
 "to specified domain id. (Does not affect\n"
 "xenpv machine type).\n",
 QEMU_ARCH_ALL)
+DEF("xen-stubdom", 0, QEMU_OPTION_xen_stubdom,
+"-xen-stubdomspecify QEMU is running in a stubdom, so certain\n"
+"behavior changes. (Does not affect xenpv machine 
type).\n",
+QEMU_ARCH_ALL)
 STEXI
 @item -xen-domid @var{id}
 @findex -xen-domid
@@ -3396,6 +3400,9 @@ Attach to existing xen domain.
 libxl will use this when starting QEMU (XEN only).
 @findex -xen-domid-restrict
 Restrict set of available xen operations to specified domain id (XEN only).
+@findex -xen-stubdom
+@item -xen-stubdom
+Run qemu in stubdom-mode (XEN only).
 ETEXI
 
 DEF("no-reboot", 0, QEMU_OPTION_no_reboot, \
diff --git a/vl.c b/vl.c
index 4a350de5cd..0d04319d9b 100644
--- a/vl.c
+++ b/vl.c
@@ -206,6 +206,7 @@ bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 bool xen_domid_restrict;
+bool xen_stubdom;
 
 static int has_defaults = 1;
 static int default_serial = 1;
@@ -3796,6 +3797,13 @@ int main(int argc, char **argv, char **envp)
 }
 xen_domid_restrict = true;
 break;
+case QEMU_OPTION_xen_stubdom:
+if (!(xen_available())) {
+error_report("Option not supported for this target");
+exit(1);
+}
+xen_stubdom = true;
+break;
 case QEMU_OPTION_trace:
 g_free(trace_file);
 trace_file = trace_opt_parse(optarg);
-- 
2.20.1

[Qemu-devel] [PATCH 3/6] xen: Skip backend initialization for stubdom

[Jason Andryuk]

When QEMU is running in a stubdom, it does not provide any
Paravirtualized backends.  Those still run in dom0 or another driver
domain.  Therefore we skip backend initialization (xen_bus_init and
xen_be_init) for the stubdom case.

Original patch by Anthony PERARD 

Signed-off-by: Jason Andryuk 
---
 hw/i386/xen/xen-hvm.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index c20c4b27f6..4b62f070cb 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1485,8 +1485,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 QLIST_INIT(&state->dev_list);
 device_listener_register(&state->device_listener);
 
-xen_bus_init();
-
 xenstore = xs_open(0);
 if (!xenstore) {
 error_report("Can't connect to xenstored");
@@ -1495,12 +1493,16 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 
 qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
 
-/* Initialize backend core & drivers */
-if (xen_be_init() != 0) {
-error_report("xen backend core setup failed");
-goto err;
+if (!xen_stubdom_enabled()) {
+xen_bus_init();
+
+/* Initialize backend core & drivers */
+if (xen_be_init() != 0) {
+error_report("xen backend core setup failed");
+goto err;
+}
+xen_be_register_common();
 }
-xen_be_register_common();
 
 QLIST_INIT(&xen_physmap);
 xen_read_physmap(state);
-- 
2.20.1

[Qemu-devel] [PATCH 0/6] Xen stubdom support

[Jason Andryuk]

Xen supports running QEMU in a dedicated service vm - a stub domain or
stubdom.  QEMU is then isolated outside of the privileged Domain-0.

When running in a stubdom, there are a few changes needed for QEMU.  On
older Xen versions, the default ioreq server needs to have the stubdom's
domid specified.  The stubdom doesn't run PV backends, so that
initialization code can be skipped.  Stubdom's don't support MSI-X, so
that PCI capability must be hidden from passed through devices.

Stubdom mode is enabled by the new -xen-stubdom flag.

Jason Andryuk (5):
  xen: Introduce -xen-stubdom option
  xen: Move xenstore initialization to common location
  xen: Skip backend initialization for stubdom
  xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen
  xen-pt: Hide MSI-X from xen stubdoms

Simon Gaiser (1):
  xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

 hw/i386/xen/xen-hvm.c   | 22 --
 hw/xen/xen-legacy-backend.c |  8 
 hw/xen/xen_pt.c | 10 +++---
 hw/xen/xen_pt_config_init.c |  3 +++
 include/hw/xen/xen.h|  6 ++
 include/hw/xen/xen_common.h |  5 +
 qemu-options.hx |  7 +++
 vl.c|  8 
 8 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

From: Simon Gaiser 

If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
an address which is not page aligned. This breaks the memory mapping via
xc_domain_memory_mapping since this function is page based and the
"offset" is therefore lost.

Without this patch you will see error like this in the stubdom log:

  [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. 
@0x0004

QubesOS/qubes-issues#2849

Signed-off-by: Simon Gaiser 
Signed-off-by: Jason Andryuk 
---
 hw/xen/xen_pt.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 5539d56c3a..7f680442ee 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -449,9 +449,10 @@ static int xen_pt_register_regions(XenPCIPassthroughState 
*s, uint16_t *cmd)
 /* Register PIO/MMIO BARs */
 for (i = 0; i < PCI_ROM_SLOT; i++) {
 XenHostPCIIORegion *r = &d->io_regions[i];
+pcibus_t r_size = r->size;
 uint8_t type;
 
-if (r->base_addr == 0 || r->size == 0) {
+if (r->base_addr == 0 || r_size == 0) {
 continue;
 }
 
@@ -469,15 +470,18 @@ static int xen_pt_register_regions(XenPCIPassthroughState 
*s, uint16_t *cmd)
 type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
 }
 *cmd |= PCI_COMMAND_MEMORY;
+
+/* Round up to a full page for the hypercall. */
+r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
 }
 
 memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
-  "xen-pci-pt-bar", r->size);
+  "xen-pci-pt-bar", r_size);
 pci_register_bar(&s->dev, i, type, &s->bar[i]);
 
 XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
" base_addr=0x%08"PRIx64" type: %#x)\n",
-   i, r->size, r->base_addr, type);
+   i, r_size, r->base_addr, type);
 }
 
 /* Register expansion ROM address */
-- 
2.20.1

[Qemu-devel] Stable 2.12.1 Request: "ccid-card-passthru: fix regression in realize()"

[Jason Andryuk]

I'd like to request this commit for QEMU 2.12.1 stable backport:

commit e58d64a16abc "ccid-card-passthru: fix regression in realize()"

The patch fixes a VM start failure which I experience on 2.12.0.
Patch inlined below

Thanks,
Jason

>From e58d64a16abc2304c4dcb644411eb9580bf63b1e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= 
Date: Tue, 15 May 2018 17:30:39 +0200
Subject: [PATCH] ccid-card-passthru: fix regression in realize()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since cc847bfd16d894fd8c1a2ce25f31772f6cdbbc74, CCID card-passthru
fails to intialize, because it changed a debug line to an error,
probably by mistake. Change it back to a DPRINTF debug.

(solves Boxes creating VM with smartcard passthru failing to start)

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20180515153039.27514-1-marcandre.lur...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/ccid-card-passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 7684db0cb3..25fb19b0d7 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -345,7 +345,7 @@ static void passthru_realize(CCIDCardState *base,
Error **errp)
 card->vscard_in_pos = 0;
 card->vscard_in_hdr = 0;
 if (qemu_chr_fe_backend_connected(&card->cs)) {
-error_setg(errp, "ccid-card-passthru: initing chardev");
+DPRINTF(card, D_INFO, "ccid-card-passthru: initing chardev");
 qemu_chr_fe_set_handlers(&card->cs,
 ccid_card_vscard_can_read,
 ccid_card_vscard_read,

[Qemu-devel] [PATCH] ccid: Fix dwProtocols advertisement of T=0

[Jason Andryuk]

Commit d7d218ef02d87c637d20d64da8f575d434ff6f78 attempted to change
dwProtocols to only advertise support for T=0 and not T=1.  The change
was incorrect as it changed 0x0003 to 0x0001.

lsusb -v in a linux guest shows:
"dwProtocols 65536  (Invalid values detected)", though the
smart card could still be accessed.  Windows 7 does not detect inserted
smart cards and logs the the following Error in the Event Logs:

Source: Smart Card Service
Event ID: 610
Smart Card Reader 'QEMU QEMU USB CCID 0' rejected IOCTL SET_PROTOCOL:
Incorrect function. If this error persists, your smart card or reader
may not be functioning correctly

Command Header: 03 00 00 00

Setting to 0x0001 fixes the Windows issue.

Signed-off-by: Jason Andryuk 
Cc: qemu-sta...@nongnu.org
---
 hw/usb/dev-smartcard-reader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index e6468057a0..cabb564788 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -329,8 +329,8 @@ static const uint8_t qemu_ccid_descriptor[] = {
  */
 0x07,   /* u8  bVoltageSupport; 01h - 5.0v, 02h - 3.0, 03 - 1.8 */
 
-0x00, 0x00, /* u32 dwProtocols;  .  = h.*/
-0x01, 0x00, /* : 0001h = Protocol T=0, 0002h = Protocol T=1 */
+0x01, 0x00, /* u32 dwProtocols;  .  = h.*/
+0x00, 0x00, /* : 0001h = Protocol T=0, 0002h = Protocol T=1 */
 /* u32 dwDefaultClock; in kHZ (0x0fa0 is 4 MHz) */
 0xa0, 0x0f, 0x00, 0x00,
 /* u32 dwMaximumClock; */
-- 
2.14.3