From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UudlU-0007Tu-KT for qemu-devel@nongnu.org; Thu, 04 Jul 2013 03:15:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UudlT-00082K-5n for qemu-devel@nongnu.org; Thu, 04 Jul 2013 03:15:40 -0400 Received: from cantor2.suse.de ([195.135.220.15]:34009 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UudlS-000828-QB for qemu-devel@nongnu.org; Thu, 04 Jul 2013 03:15:39 -0400 Message-ID: <51D52116.8040104@suse.de> Date: Thu, 04 Jul 2013 09:15:34 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1372850397-29947-1-git-send-email-paul.durrant@citrix.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] Xen PV Device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Paul Durrant , Stefano Stabellini , qemu-devel@nongnu.org, xen-devel@lists.xen.org Am 03.07.2013 18:37, schrieb Stefano Stabellini: > On Wed, 3 Jul 2013, Paul Durrant wrote: >> This patch introduces a new Xen PV PCI device which will act as a new >> binding point for PV drivers for Xen. >> The device has parameterized vendor-id, device-id and revision to allo= w to >> be configured as a binding point for any vendor's PV drivers. >> >> Signed-off-by: Paul Durrant >> Cc: Stefano Stabellini >> --- >> hw/xen/Makefile.objs | 1 + >> hw/xen/xen_pvdevice.c | 131 +++++++++++++++++++++++++++++++++++++= +++++++++ >> include/hw/pci/pci_ids.h | 5 +- >> trace-events | 4 ++ >> 4 files changed, 139 insertions(+), 2 deletions(-) >> create mode 100644 hw/xen/xen_pvdevice.c >> >> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs >> index 2017560..fd88003 100644 >> --- a/hw/xen/Makefile.objs >> +++ b/hw/xen/Makefile.objs >> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_XEN_BACKEND) +=3D xen_backend.o xe= n_devconfig.o >> obj-$(CONFIG_XEN_I386) +=3D xen_platform.o xen_apic.o >> obj-$(CONFIG_XEN_PCI_PASSTHROUGH) +=3D xen-host-pci-device.o >> obj-$(CONFIG_XEN_PCI_PASSTHROUGH) +=3D xen_pt.o xen_pt_config_init.o = xen_pt_msi.o >> +obj-$(CONFIG_XEN) +=3D xen_pvdevice.o >> diff --git a/hw/xen/xen_pvdevice.c b/hw/xen/xen_pvdevice.c >> new file mode 100644 >> index 0000000..dbc4bf5 >> --- /dev/null >> +++ b/hw/xen/xen_pvdevice.c >> @@ -0,0 +1,131 @@ >> +/* Copyright (c) Citrix Systems Inc. >> + * All rights reserved. >=20 > Like Anthony wrote before, All rights reserved contradicts what's > written below. > Aside from this, it looks OK to me. >=20 > I would like to see the libxl side patch. > Also it would be nice to have an ack from Andreas or another QOM expert= . >>From a QOM view it looks fine now. :) Thanks for inquiring. Some other comments though: * Now that it no longer depends on TARGET_PAGE_SIZE, is it possible to use common-obj-$(CONFIG_XEN)? Then it would build only once rather than separately for i386 and x86_64 and any future Xen platforms (e.g., arm). * It looks as if the MMIO functions were renamed - the arguments no longer align. That could be edited before you apply the patch to your queue if there's nothing else - then feel free to add my Reviewed-by independent of the other issue. * Paolo had asked for new MemoryRegions not to include the device name - can be renamed once they get the owner field though (not merged yet). Don't have a better suggestion handy. Also Paul, by my count this is [PATCH v4] - please use --subject-prefix=3D"PATCH v5" if you respin and include the change log either below "---" or in a cover letter. We prefer to see it for patch review but not in Git commit history. Similarly, "Introduce a new Xen PV device..." would elegantly avoid reading "This patch..." after it's been committed. ;) Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg