[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Xen PV Device
From: |
Paul Durrant |
Subject: |
Re: [Qemu-devel] [PATCH] Xen PV Device |
Date: |
Thu, 4 Jul 2013 08:14:00 +0000 |
> -----Original Message-----
> >
> > Like Anthony wrote before, All rights reserved contradicts what's
> > written below.
Like I said, it's part of all BSD licenses that I can find. It's certainly in
the template on the OSI website and the FreeBSD license for instance.
> > 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).
Sure, that sounds sensible.
> * 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.
Thanks.
> * 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.
>
I guess this can be fixed up later.
> 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.
Ok. I was unsure what to do since this device was under a different name so I
opted to reset the version back to 1. I'll call the next one v5 as you suggest.
I'm still finding my way with git so thanks for the tips.
> Similarly, "Introduce a new Xen PV device..." would elegantly avoid
> reading "This patch..." after it's been committed. ;)
>
Sure. Good point.
Paul