[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common




On 22.09.20 09:33, Jan Beulich wrote:

Hi Jan

On 21.09.2020 21:02, Oleksandr wrote:
On 14.09.20 17:17, Jan Beulich wrote:
On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
--- /dev/null
+++ b/xen/include/xen/ioreq.h
@@ -0,0 +1,82 @@
+/*
+ * ioreq.h: Hardware virtual machine assist interface definitions.
+ *
+ * Copyright (c) 2016 Citrix Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __IOREQ_H__
+#define __IOREQ_H__
__XEN_IOREQ_H__ please.
ok


+#include <xen/sched.h>
+
+#include <asm/hvm/ioreq.h>
Is this include really needed here (i.e. by the code further down in
the file, and hence by everyone including this header), or rather
just in a few specific .c files?
I think, just in few specific .c files. Which are x86/hvm/ioreq.c and
common/ioreq.c now and several other files later on (x86/hvm/dm.c,
arm/io.c, etc)
Shall I include that header in these files instead?
Yes please, and please take this as a common guideline. While
private headers are often used to include things needed by all
of the (typically few) users of the header, non-private ones
shouldn't create unnecessary dependencies on other headers. As
you've said further up - you did run into hard to resolve
header dependencies yourself, and the practice of including
headers without strict need is one of the reasons of such
problems.

Got it.



+#define GET_IOREQ_SERVER(d, id) \
+    (d)->arch.hvm.ioreq_server.server[id]
arch.hvm.* feels like a layering violation when used in this header.
Got it. The only reason why GET_IOREQ_SERVER is here is inline
get_ioreq_server(). I will make it non-inline and move both to
common/ioreq.c.
Which won't make the layering violation go away. It's still
common rather than per-arch code then. As suggested elsewhere,
I think the whole ioreq_server struct wants to move into
struct domain itself, perhaps inside a new #ifdef (iirc one of
the patches introduces a suitable Kconfig option).
Well, your advise regarding ioreq_server sounds reasonable, but the common ioreq.c still will have other *arch.hvm.* for both vcpu and domain. So looks like other involved structs should be moved into *common* struct domain/vcpu itself, correct? Some of them could be moved easily since contain the same fields (arch.hvm.ioreq_gfn), but some of them couldn't and seems to require to pull a lot of changes to the Xen code (arch.hvm.params, arch.hvm.hvm_io), I am afraid.
Or I missed something?


This goes
alongside my suggestion to drop the "hvm" prefixes and infixes
from involved function names.
Well, I assume this request as well as the request above should be addressed in the follow-up patches, as we want to keep the code movement in current patch as (almost) verbatim copy,
Am I correct?

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.