[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.
On 4/28/2016 8:06 PM, Wei Liu wrote:
On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
On 28/04/16 12:59, Wei Liu wrote:
On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
Thanks Jan. And I admire your rigorous thought. :)
On 4/28/2016 6:57 PM, Jan Beulich wrote:
On 28.04.16 at 12:42, <george.dunlap@xxxxxxxxxx> wrote:
On 28/04/16 11:22, Jan Beulich wrote:
On 28.04.16 at 10:29, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
@@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE_PARAM(void) arg)
[HVMMEM_ram_rw] = p2m_ram_rw,
[HVMMEM_ram_ro] = p2m_ram_ro,
[HVMMEM_mmio_dm] = p2m_mmio_dm,
- [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
+ [HVMMEM_unused] = p2m_invalid
Why don't you simply delete the old line, without replacement?
Well, I did not delete the old line, because in my coming patch(the
p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
against HVMMEN_unused later in this routine appear in that patch.
That might have been slightly cleaner; but we're going to have to put it
back as soon as the development window opens anyway, so I don't really
see the point of going through the effort of respinning the patch again.
Would you be willing to ack this version anyway?
I have no problem doing so (and in fact I have it on my to by
committed list already), it is just looked slightly confusing (and
I had already typed half a reply that this isn't what was discussed
until I properly looked at the next hunk), and hence I wanted to
understand the motivation. And btw., I'm not convinced it would
need to be put there anyway later: I don't view the used
mechanism as a good (read: extensible) one to deal with what
would be holes in the array above. Indeed we can't leave them
uninitialized (as that would mean p2m_ram_rw), but I think we
should better find a way to initialize _all_ unused slots without
requiring an initializer for each of them. Sadly the desire to allow
compilation with clang prohibits the most natural solution:
static const p2m_type_t memtype[] = {
[0 ... <upper-bound> - 1] = p2m_invalid,
Not sure if this will compile? Can have a try. :)
To answer your question this can compile with gcc but not probably not
with clang. This syntax is gcc extension.
See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
That syntax works in Clang, but will subsequent entries in the list will
suffer a -Werror,-Winitializer-overrides and fail to compile.
This can easily be fixed :-)
[ 0 ... <first-upper-bound> ] = p2m_inavlid;
[ <second-lower-bound> ... <second-upper-bound> ] = p2m_invalid;
But I'm not sure whether you guys think this is pretty or ugly.
Thanks for your information, Wei. :)
But <first-upper-bound> and <second-lower-bound> ... <second-upper
bound> seems to be holes in this array.
I'm still confused why do we need this, especially at such critical
moment. IIUC HVMMEM type is used to get/set mem type, why would someone
define a HVMMEM type but not use it here?
I know HVMMEM_mmio_write_dm is unused now, but I do not think this
should be a common case in the future.
Frankly, I had thought to remove the HVMMEM_unused in the set_mem_type
code, I choose not to do so, because I do not wanna the check of
a.hvmmem_type against HVMMEN_unused to pop in my next patch, and I do
not think keeping this will harm any functionality. :)
Wei.
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
