Re: [Xen-devel] [PATCH] Xen PV Device

[Andreas Färber <afaerber@xxxxxxx>]

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 allow to
>> be configured as a binding point for any vendor's PV drivers.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>> ---
>>  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) += xen_backend.o 
>> xen_devconfig.o
>>  obj-$(CONFIG_XEN_I386) += xen_platform.o xen_apic.o
>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o 
>> xen_pt_msi.o
>> +obj-$(CONFIG_XEN) += 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.
> 
> Like Anthony wrote before, All rights reserved contradicts what's
> written below.
> Aside from this, it looks OK to me.
> 
> 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="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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel