diff mbox series

[7/7] VT-d: move {,un}map_vtd_domain_page()

Message ID 22e7036c-cf49-4160-bd26-fbba6b67ff5d@suse.com (mailing list archive)
State New, archived
Headers show
Series VT-d: SATC handling and ATS tidying | expand

Commit Message

Jan Beulich Feb. 5, 2024, 1:57 p.m. UTC 
..., thus allowing them to become static. There's nothing x86-specific
about these functions anyway.

Since only the "iommu_inclusive_mapping" parameter declaration would be
left in the file, move that as well. There's nothing VT-d specific about
it (anymore?): "dom0-iommu=map-inclusive" is similarly generic, and
documentation also doesn't say anything.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Roger Pau Monné Feb. 9, 2024, 8:39 a.m. UTC | #1 
On Mon, Feb 05, 2024 at 02:57:30PM +0100, Jan Beulich wrote:
> ..., thus allowing them to become static. There's nothing x86-specific
> about these functions anyway.
> 
> Since only the "iommu_inclusive_mapping" parameter declaration would be
> left in the file, move that as well. There's nothing VT-d specific about
> it (anymore?): "dom0-iommu=map-inclusive" is similarly generic, and
> documentation also doesn't say anything.

Hm, I guess documentation should at least say that
iommu_inclusive_mapping is x86 specific, because it's not parsed on
Arm and hence might give the wrong impression that it's actually
acknowledged there.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Albeit I think it would be better to put the parsing in generic
iommu.c, so that the option gets parsed on Arm and
arch_iommu_hwdom_init() can print a warning message about it not
supported on Arm.

Thanks, Roger.
Jan Beulich Feb. 12, 2024, 9:46 a.m. UTC | #2 
On 09.02.2024 09:39, Roger Pau Monné wrote:
> On Mon, Feb 05, 2024 at 02:57:30PM +0100, Jan Beulich wrote:
>> ..., thus allowing them to become static. There's nothing x86-specific
>> about these functions anyway.
>>
>> Since only the "iommu_inclusive_mapping" parameter declaration would be
>> left in the file, move that as well. There's nothing VT-d specific about
>> it (anymore?): "dom0-iommu=map-inclusive" is similarly generic, and
>> documentation also doesn't say anything.
> 
> Hm, I guess documentation should at least say that
> iommu_inclusive_mapping is x86 specific, because it's not parsed on
> Arm and hence might give the wrong impression that it's actually
> acknowledged there.

In v2 I'm adding "(x86)" there.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Albeit I think it would be better to put the parsing in generic
> iommu.c, so that the option gets parsed on Arm and
> arch_iommu_hwdom_init() can print a warning message about it not
> supported on Arm.

Hmm, I would have considered doing things the other way around - make
that part of parsing in parse_dom0_iommu_param() x86-only. I would
feel odd to introduce an option to Arm, just to be able to report
that it's unsupported. The more when generic option parsing code will
already log unrecognized options (sadly such log messages aren't seen
in the serial log, for being issued too early). But let's ask Arm
folks what they'd prefer, by adding all of them to To:.

Jan
Roger Pau Monné Feb. 12, 2024, 10:11 a.m. UTC | #3 
On Mon, Feb 12, 2024 at 10:46:55AM +0100, Jan Beulich wrote:
> On 09.02.2024 09:39, Roger Pau Monné wrote:
> > On Mon, Feb 05, 2024 at 02:57:30PM +0100, Jan Beulich wrote:
> >> ..., thus allowing them to become static. There's nothing x86-specific
> >> about these functions anyway.
> >>
> >> Since only the "iommu_inclusive_mapping" parameter declaration would be
> >> left in the file, move that as well. There's nothing VT-d specific about
> >> it (anymore?): "dom0-iommu=map-inclusive" is similarly generic, and
> >> documentation also doesn't say anything.
> > 
> > Hm, I guess documentation should at least say that
> > iommu_inclusive_mapping is x86 specific, because it's not parsed on
> > Arm and hence might give the wrong impression that it's actually
> > acknowledged there.
> 
> In v2 I'm adding "(x86)" there.

Maybe part of this should be done as a separate patch so it can be
backported to stable branches?

> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > Albeit I think it would be better to put the parsing in generic
> > iommu.c, so that the option gets parsed on Arm and
> > arch_iommu_hwdom_init() can print a warning message about it not
> > supported on Arm.
> 
> Hmm, I would have considered doing things the other way around - make
> that part of parsing in parse_dom0_iommu_param() x86-only. I would
> feel odd to introduce an option to Arm, just to be able to report
> that it's unsupported. The more when generic option parsing code will
> already log unrecognized options (sadly such log messages aren't seen
> in the serial log, for being issued too early). But let's ask Arm
> folks what they'd prefer, by adding all of them to To:.

FWIW, that (moving part of the parsing of parse_dom0_iommu_param() to
x86-only code) would also be fine.  As long as both
dom0-iommu=map-inclusive and iommu_inclusive_mapping are handled
equally and the documentation is updated to reflect that.

Thanks, Roger.
diff mbox series

Patch

--- a/xen/drivers/passthrough/vtd/Makefile
+++ b/xen/drivers/passthrough/vtd/Makefile
@@ -1,5 +1,3 @@ 
-obj-$(CONFIG_X86) += x86/
-
 obj-y += iommu.o
 obj-y += dmar.o
 obj-y += utils.o
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -21,6 +21,7 @@ 
 #define _VTD_EXTERN_H_
 
 #include "dmar.h"
+#include <xen/domain_page.h>
 #include <xen/keyhandler.h>
 
 #define VTDPREFIX "[VT-D]"
@@ -68,8 +69,6 @@  struct acpi_rhsa_unit *drhd_to_rhsa(cons
 
 uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node);
 void free_pgtable_maddr(u64 maddr);
-void *map_vtd_domain_page(u64 maddr);
-void unmap_vtd_domain_page(const void *va);
 int domain_context_mapping_one(struct domain *domain, struct vtd_iommu *iommu,
                                uint8_t bus, uint8_t devfn,
                                const struct pci_dev *pdev, domid_t domid,
@@ -79,6 +78,16 @@  int domain_context_unmap_one(struct doma
 int cf_check intel_iommu_get_reserved_device_memory(
     iommu_grdm_t *func, void *ctxt);
 
+static inline void *map_vtd_domain_page(paddr_t maddr)
+{
+    return map_domain_page(_mfn(paddr_to_pfn(maddr)));
+}
+
+static inline void unmap_vtd_domain_page(const void *va)
+{
+    unmap_domain_page(va);
+}
+
 unsigned int cf_check io_apic_read_remap_rte(
     unsigned int apic, unsigned int reg);
 void cf_check io_apic_write_remap_rte(
--- a/xen/drivers/passthrough/vtd/x86/Makefile
+++ /dev/null
@@ -1 +0,0 @@ 
-obj-y += vtd.o
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ /dev/null
@@ -1,48 +0,0 @@ 
-/*
- * Copyright (c) 2008, Intel Corporation.
- *
- * 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/>.
- *
- * Copyright (C) Allen Kay <allen.m.kay@intel.com>
- * Copyright (C) Weidong Han <weidong.han@intel.com>
- */
-
-#include <xen/param.h>
-#include <xen/sched.h>
-#include <xen/softirq.h>
-#include <xen/domain_page.h>
-#include <asm/paging.h>
-#include <xen/iommu.h>
-#include <xen/irq.h>
-#include <xen/numa.h>
-#include <asm/fixmap.h>
-#include "../iommu.h"
-#include "../dmar.h"
-#include "../vtd.h"
-#include "../extern.h"
-
-/*
- * iommu_inclusive_mapping: when set, all memory below 4GB is included in dom0
- * 1:1 iommu mappings except xen and unusable regions.
- */
-boolean_param("iommu_inclusive_mapping", iommu_hwdom_inclusive);
-
-void *map_vtd_domain_page(u64 maddr)
-{
-    return map_domain_page(_mfn(paddr_to_pfn(maddr)));
-}
-
-void unmap_vtd_domain_page(const void *va)
-{
-    unmap_domain_page(va);
-}
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -19,6 +19,7 @@ 
 #include <xen/paging.h>
 #include <xen/guest_access.h>
 #include <xen/event.h>
+#include <xen/param.h>
 #include <xen/softirq.h>
 #include <xen/vm_event.h>
 #include <xsm/xsm.h>
@@ -36,6 +37,12 @@  bool __initdata iommu_superpages = true;
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
 
+/*
+ * iommu_inclusive_mapping: When set, all memory below 4GB is included in dom0
+ * 1:1 iommu mappings except xen and unusable regions.
+ */
+boolean_param("iommu_inclusive_mapping", iommu_hwdom_inclusive);
+
 #ifdef CONFIG_PV
 /* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
 bool __read_mostly untrusted_msi;