Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory
On 18.07.2022 13:03, Marek Marczykowski-Górecki wrote: > On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote: >> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote: >>> +}; >>> +static unsigned int __initdata nr_extra_reserved_ranges; >>> +static struct extra_reserved_range __initdata >>> extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES]; >> >> Overly long line. > > I can't find what coding style dictate in such case. Should the second > line be indented (and how much)? I'd recommend static struct extra_reserved_range __initdata extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES]; Jan
Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory
On 18.07.2022 12:53, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -651,6 +651,46 @@ bool_t iommu_has_feature(struct domain *d, enum
>>> iommu_feature feature)
>>> return is_iommu_enabled(d) && test_bit(feature,
>>> dom_iommu(d)->features);
>>> }
>>>
>>> +#define MAX_EXTRA_RESERVED_RANGES 20
>>> +struct extra_reserved_range {
>>> +xen_pfn_t start;
>>
>> Why not unsigned long?
>
> Isn't this the correct type for the page number?
In the public interface - yes. But internally we (almost) universally
use unsigned long. xen_pfn_t is inefficient to use on Arm32 (and then
presumably also other future 32-bit counterparts of 64-bit
architectures, inheriting the choices made for Arm and differing from
what we have on x86).
Jan
Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory
On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote: > On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote: > > +}; > > +static unsigned int __initdata nr_extra_reserved_ranges; > > +static struct extra_reserved_range __initdata > > extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES]; > > Overly long line. I can't find what coding style dictate in such case. Should the second line be indented (and how much)? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab signature.asc Description: PGP signature
Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory
On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -651,6 +651,46 @@ bool_t iommu_has_feature(struct domain *d, enum
> > iommu_feature feature)
> > return is_iommu_enabled(d) && test_bit(feature,
> > dom_iommu(d)->features);
> > }
> >
> > +#define MAX_EXTRA_RESERVED_RANGES 20
> > +struct extra_reserved_range {
> > +xen_pfn_t start;
>
> Why not unsigned long?
Isn't this the correct type for the page number?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
signature.asc
Description: PGP signature
Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory
On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> Add API similar to rmrr= and ivmd= arguments, but in a common code. This
> will allow drivers to register reserved memory regardless of the IOMMU
> vendor.
> The direct reason for this API is xhci-dbc console driver (aka xue),
> that needs to use DMA. But future change may unify command line
> arguments for user-supplied reserved memory, and it may be useful for
> other drivers in the future too.
I take it that I'll understand the purpose of this when making it to
later patches.
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -651,6 +651,46 @@ bool_t iommu_has_feature(struct domain *d, enum
> iommu_feature feature)
> return is_iommu_enabled(d) && test_bit(feature, dom_iommu(d)->features);
> }
>
> +#define MAX_EXTRA_RESERVED_RANGES 20
> +struct extra_reserved_range {
> +xen_pfn_t start;
Why not unsigned long?
> +xen_ulong_t nr;
Why not unsigned int or unsigned long?
> +u32 sbdf;
uint32_t please.
All these type related remarks apply elsewhere as well; others
below also apply wherever else applicable.
> +};
> +static unsigned int __initdata nr_extra_reserved_ranges;
> +static struct extra_reserved_range __initdata
> extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
Overly long line.
> +int iommu_add_extra_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
> u32 sbdf)
> +{
> +unsigned int idx;
> +
> +if ( nr_extra_reserved_ranges >= MAX_EXTRA_RESERVED_RANGES )
> +return -ENOMEM;
> +
> +idx = nr_extra_reserved_ranges++;
> +extra_reserved_ranges[idx].start = start;
> +extra_reserved_ranges[idx].nr = nr;
> +extra_reserved_ranges[idx].sbdf = sbdf;
> +return 0;
> +}
Blank line please before final return statement.
Jan
[PATCH v2 5/9] IOMMU: add common API for device reserved memory
Add API similar to rmrr= and ivmd= arguments, but in a common code. This
will allow drivers to register reserved memory regardless of the IOMMU
vendor.
The direct reason for this API is xhci-dbc console driver (aka xue),
that needs to use DMA. But future change may unify command line
arguments for user-supplied reserved memory, and it may be useful for
other drivers in the future too.
This commit just introduces an API, subsequent patches will plug it in
appropriate places. The reserved memory ranges needs to be saved
locally, because at the point when they are collected, Xen doesn't know
yet which IOMMU driver will be used.
Signed-off-by: Marek Marczykowski-Górecki
---
xen/drivers/passthrough/iommu.c | 40 ++-
xen/include/xen/iommu.h | 11 +-
2 files changed, 51 insertions(+)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 75df3aa8ddaa..257691ad19ef 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -651,6 +651,46 @@ bool_t iommu_has_feature(struct domain *d, enum
iommu_feature feature)
return is_iommu_enabled(d) && test_bit(feature, dom_iommu(d)->features);
}
+#define MAX_EXTRA_RESERVED_RANGES 20
+struct extra_reserved_range {
+xen_pfn_t start;
+xen_ulong_t nr;
+u32 sbdf;
+};
+static unsigned int __initdata nr_extra_reserved_ranges;
+static struct extra_reserved_range __initdata
extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
+
+int iommu_add_extra_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
u32 sbdf)
+{
+unsigned int idx;
+
+if ( nr_extra_reserved_ranges >= MAX_EXTRA_RESERVED_RANGES )
+return -ENOMEM;
+
+idx = nr_extra_reserved_ranges++;
+extra_reserved_ranges[idx].start = start;
+extra_reserved_ranges[idx].nr = nr;
+extra_reserved_ranges[idx].sbdf = sbdf;
+return 0;
+}
+
+int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+unsigned int idx;
+int ret;
+
+for ( idx = 0; idx < nr_extra_reserved_ranges; idx++ )
+{
+ret = func(extra_reserved_ranges[idx].start,
+ extra_reserved_ranges[idx].nr,
+ extra_reserved_ranges[idx].sbdf,
+ ctxt);
+if ( ret < 0 )
+return ret;
+}
+return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 79529adf1fa5..7640f40d77b5 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -297,6 +297,17 @@ struct iommu_ops {
#endif
};
+/*
+ * To be called by Xen internally, to register extra RMRR/IVMD ranges.
+ * Needs to be called before IOMMU initialization.
+ */
+extern int iommu_add_extra_reserved_device_memory(xen_pfn_t start, xen_ulong_t
nr, u32 sbdf);
+/*
+ * To be called by specific IOMMU driver during initialization,
+ * to fetch ranges registered with iommu_add_extra_reserved_device_memory().
+ */
+extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, void
*ctxt);
+
#include
#ifndef iommu_call
--
git-series 0.9.1
