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. >>> +#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). This goes alongside my suggestion to drop the "hvm" prefixes and infixes from involved function names. Jan
