| Message ID | 2538c56f6881e4e2a3e5cee3cbdeebb88a59ccb5.1657121519.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
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
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?
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)?
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
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
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 <asm/iommu.h> #ifndef iommu_call
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 <marmarek@invisiblethingslab.com> --- xen/drivers/passthrough/iommu.c | 40 ++++++++++++++++++++++++++++++++++- xen/include/xen/iommu.h | 11 +++++++++- 2 files changed, 51 insertions(+)