Re: [PATCH v2] hvc/xen: lock console list traversal
On Wed, 30 Nov 2022, Roger Pau Monne wrote:
> The currently lockless access to the xen console list in
> vtermno_to_xencons() is incorrect, as additions and removals from the
> list can happen anytime, and as such the traversal of the list to get
> the private console data for a given termno needs to happen with the
> lock held. Note users that modify the list already do so with the
> lock taken.
>
> Adjust current lock takers to use the _irq{save,restore} helpers,
> since the context in which vtermno_to_xencons() is called can have
> interrupts disabled. Use the _irq{save,restore} set of helpers to
> switch the current callers to disable interrupts in the locked region.
> I haven't checked if existing users could instead use the _irq
> variant, as I think it's safer to use _irq{save,restore} upfront.
>
> While there switch from using list_for_each_entry_safe to
> list_for_each_entry: the current entry cursor won't be removed as
> part of the code in the loop body, so using the _safe variant is
> pointless.
>
> Fixes: 02e19f9c7cac ('hvc_xen: implement multiconsole support')
> Signed-off-by: Roger Pau Monné
Reviewed-by: Stefano Stabellini
> ---
> Changes since v1:
> - Switch current lock users to disable interrupts in the locked
>region.
> ---
> drivers/tty/hvc/hvc_xen.c | 46 ---
> 1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index e63c1761a361..d9d023275328 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -53,17 +53,22 @@ static DEFINE_SPINLOCK(xencons_lock);
>
> static struct xencons_info *vtermno_to_xencons(int vtermno)
> {
> - struct xencons_info *entry, *n, *ret = NULL;
> + struct xencons_info *entry, *ret = NULL;
> + unsigned long flags;
>
> - if (list_empty(&xenconsoles))
> - return NULL;
> + spin_lock_irqsave(&xencons_lock, flags);
> + if (list_empty(&xenconsoles)) {
> + spin_unlock_irqrestore(&xencons_lock, flags);
> + return NULL;
> + }
>
> - list_for_each_entry_safe(entry, n, &xenconsoles, list) {
> + list_for_each_entry(entry, &xenconsoles, list) {
> if (entry->vtermno == vtermno) {
> ret = entry;
> break;
> }
> }
> + spin_unlock_irqrestore(&xencons_lock, flags);
>
> return ret;
> }
> @@ -234,7 +239,7 @@ static int xen_hvm_console_init(void)
> {
> int r;
> uint64_t v = 0;
> - unsigned long gfn;
> + unsigned long gfn, flags;
> struct xencons_info *info;
>
> if (!xen_hvm_domain())
> @@ -270,9 +275,9 @@ static int xen_hvm_console_init(void)
> goto err;
> info->vtermno = HVC_COOKIE;
>
> - spin_lock(&xencons_lock);
> + spin_lock_irqsave(&xencons_lock, flags);
> list_add_tail(&info->list, &xenconsoles);
> - spin_unlock(&xencons_lock);
> + spin_unlock_irqrestore(&xencons_lock, flags);
>
> return 0;
> err:
> @@ -296,6 +301,7 @@ static int xencons_info_pv_init(struct xencons_info
> *info, int vtermno)
> static int xen_pv_console_init(void)
> {
> struct xencons_info *info;
> + unsigned long flags;
>
> if (!xen_pv_domain())
> return -ENODEV;
> @@ -312,9 +318,9 @@ static int xen_pv_console_init(void)
> /* already configured */
> return 0;
> }
> - spin_lock(&xencons_lock);
> + spin_lock_irqsave(&xencons_lock, flags);
> xencons_info_pv_init(info, HVC_COOKIE);
> - spin_unlock(&xencons_lock);
> + spin_unlock_irqrestore(&xencons_lock, flags);
>
> return 0;
> }
> @@ -322,6 +328,7 @@ static int xen_pv_console_init(void)
> static int xen_initial_domain_console_init(void)
> {
> struct xencons_info *info;
> + unsigned long flags;
>
> if (!xen_initial_domain())
> return -ENODEV;
> @@ -337,9 +344,9 @@ static int xen_initial_domain_console_init(void)
> info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
> info->vtermno = HVC_COOKIE;
>
> - spin_lock(&xencons_lock);
> + spin_lock_irqsave(&xencons_lock, flags);
> list_add_tail(&info->list, &xenconsoles);
> - spin_unlock(&xencons_lock);
> + spin_unlock_irqrestore(&xencons_lock, flags);
>
> return 0;
> }
> @@ -394,10 +401,12 @@ static void xencons_free(struct xencons_info *info)
Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
On Wed, 30 Nov 2022, Roger Pau Monne wrote:
> The hvc machinery registers both a console and a tty device based on
> the hv ops provided by the specific implementation. Those two
> interfaces however have different locks, and there's no single locks
> that's shared between the tty and the console implementations, hence
> the driver needs to protect itself against concurrent accesses.
> Otherwise concurrent calls using the split interfaces are likely to
> corrupt the ring indexes, leaving the console unusable.
>
> Introduce a lock to xencons_info to serialize accesses to the shared
> ring. This is only required when using the shared memory console,
> concurrent accesses to the hypercall based console implementation are
> not an issue.
>
> Note the conditional logic in domU_read_console() is slightly modified
> so the notify_daemon() call can be done outside of the locked region:
> it's an hypercall and there's no need for it to be done with the lock
> held.
For domU_read_console: I don't mean to block this patch but we need to
be sure about the semantics of hv_ops.get_chars. Either it is expected
to be already locked, then we definitely shouldn't add another lock to
domU_read_console. Or it is not expected to be already locked, then we
should add the lock.
My impression is that it is expected to be already locked, but I think
we need Greg or Jiri to confirm one way or the other.
Aside from that the rest looks fine.
> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen
> console')
> Signed-off-by: Roger Pau Monné
> ---
> Changes since v1:
> - Properly initialize the introduced lock in all paths.
> ---
> drivers/tty/hvc/hvc_xen.c | 19 +--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 7c23112dc923..e63c1761a361 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -43,6 +43,7 @@ struct xencons_info {
> int irq;
> int vtermno;
> grant_ref_t gntref;
> + spinlock_t ring_lock;
> };
>
> static LIST_HEAD(xenconsoles);
> @@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
> XENCONS_RING_IDX cons, prod;
> struct xencons_interface *intf = xencons->intf;
> int sent = 0;
> + unsigned long flags;
>
> + spin_lock_irqsave(&xencons->ring_lock, flags);
> cons = intf->out_cons;
> prod = intf->out_prod;
> mb(); /* update queue values before going on */
>
> if ((prod - cons) > sizeof(intf->out)) {
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> pr_err_once("xencons: Illegal ring page indices");
> return -EINVAL;
> }
> @@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
>
> wmb(); /* write ring before updating pointer */
> intf->out_prod = prod;
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
>
> if (sent)
> notify_daemon(xencons);
> @@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char
> *buf, int len)
> int recv = 0;
> struct xencons_info *xencons = vtermno_to_xencons(vtermno);
> unsigned int eoiflag = 0;
> + unsigned long flags;
>
> if (xencons == NULL)
> return -EINVAL;
> intf = xencons->intf;
>
> + spin_lock_irqsave(&xencons->ring_lock, flags);
> cons = intf->in_cons;
> prod = intf->in_prod;
> mb(); /* get pointers before reading ring */
>
> if ((prod - cons) > sizeof(intf->in)) {
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> pr_err_once("xencons: Illegal ring page indices");
> return -EINVAL;
> }
> @@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char
> *buf, int len)
> xencons->out_cons = intf->out_cons;
> xencons->out_cons_same = 0;
> }
> + if (!recv && xencons->out_cons_same++ > 1) {
> + eoiflag = XEN_EOI_FLAG_SPURIOUS;
> + }
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> +
> if (recv) {
> notify_daemon(xencons);
> - } else if (xencons->out_cons_same++ > 1) {
> - eoiflag = XEN_EOI_FLAG_SPURIOUS;
> }
>
> xen_irq_lateeoi(xencons->irq, eoiflag);
> @@ -234,6 +245,7 @@ static int xen_hvm_console_init(void)
> info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
> + spin_lock_init(&info->ring_lock);
> } else if (info->intf != NULL) {
> /* already configured */
> return 0;
> @@ -270,6 +282,7 @@ static int xen_hvm_console_init(void)
>
> static int xencons_info_pv_init(struct xencons_info *info, int vtermno)
> {
> + spin_lock_init(&info->ring_lock);
> info
Re: [PATCH] hvc/xen: lock console list traversal
On Tue, 29 Nov 2022, Roger Pau Monne wrote:
> The currently lockless access to the xen console list in
> vtermno_to_xencons() is incorrect, as additions and removals from the
> list can happen anytime, and as such the traversal of the list to get
> the private console data for a given termno needs to happen with the
> lock held. Note users that modify the list already do so with the
> lock taken.
>
> While there switch from using list_for_each_entry_safe to
> list_for_each_entry: the current entry cursor won't be removed as
> part of the code in the loop body, so using the _safe variant is
> pointless.
>
> Fixes: 02e19f9c7cac ('hvc_xen: implement multiconsole support')
> Signed-off-by: Roger Pau Monné
> ---
> drivers/tty/hvc/hvc_xen.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index d65741983837..117dc48f980e 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -53,17 +53,22 @@ static DEFINE_SPINLOCK(xencons_lock);
>
> static struct xencons_info *vtermno_to_xencons(int vtermno)
> {
> - struct xencons_info *entry, *n, *ret = NULL;
> + struct xencons_info *entry, *ret = NULL;
> + unsigned long flags;
>
> - if (list_empty(&xenconsoles))
> - return NULL;
> + spin_lock_irqsave(&xencons_lock, flags);
If xencons_lock requires irqsave then we need to change all the
xencons_lock spinlocks to call irqsave, including the ones in
xen_hvm_console_init if they can happen at runtime.
> + if (list_empty(&xenconsoles)) {
> + spin_unlock_irqrestore(&xencons_lock, flags);
> + return NULL;
> + }
>
> - list_for_each_entry_safe(entry, n, &xenconsoles, list) {
> + list_for_each_entry(entry, &xenconsoles, list) {
> if (entry->vtermno == vtermno) {
> ret = entry;
> break;
> }
> }
> + spin_unlock_irqrestore(&xencons_lock, flags);
>
> return ret;
> }
> --
> 2.37.3
>
Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring
On Tue, 29 Nov 2022, Roger Pau Monne wrote:
> The hvc machinery registers both a console and a tty device based on
> the hv ops provided by the specific implementation. Those two
> interfaces however have different locks, and there's no single locks
> that's shared between the tty and the console implementations, hence
> the driver needs to protect itself against concurrent accesses.
> Otherwise concurrent calls using the split interfaces are likely to
> corrupt the ring indexes, leaving the console unusable.
>
> Introduce a lock to xencons_info to serialize accesses to the shared
> ring. This is only required when using the shared memory console,
> concurrent accesses to the hypercall based console implementation are
> not an issue.
>
> Note the conditional logic in domU_read_console() is slightly modified
> so the notify_daemon() call can be done outside of the locked region:
> it's an hypercall and there's no need for it to be done with the lock
> held.
>
> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen
> console')
> Signed-off-by: Roger Pau Monné
> ---
> While the write handler (domU_write_console()) is used by both the
> console and the tty ops, that's not the case for the read side
> (domU_read_console()). It's not obvious to me whether we could get
> concurrent poll calls from the poll_get_char tty hook, hence stay on
> the safe side also serialize read accesses in domU_read_console().
I think domU_read_console doesn't need it. struct hv_ops and struct
console are both already locked although independently locked.
I think we shouldn't add an unrequired lock there.
> ---
> drivers/tty/hvc/hvc_xen.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 7c23112dc923..d65741983837 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -43,6 +43,7 @@ struct xencons_info {
> int irq;
> int vtermno;
> grant_ref_t gntref;
> + spinlock_t ring_lock;
> };
>
> static LIST_HEAD(xenconsoles);
> @@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
> XENCONS_RING_IDX cons, prod;
> struct xencons_interface *intf = xencons->intf;
> int sent = 0;
> + unsigned long flags;
>
> + spin_lock_irqsave(&xencons->ring_lock, flags);
> cons = intf->out_cons;
> prod = intf->out_prod;
> mb(); /* update queue values before going on */
>
> if ((prod - cons) > sizeof(intf->out)) {
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> pr_err_once("xencons: Illegal ring page indices");
> return -EINVAL;
> }
> @@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
>
> wmb(); /* write ring before updating pointer */
> intf->out_prod = prod;
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
>
> if (sent)
> notify_daemon(xencons);
> @@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char
> *buf, int len)
> int recv = 0;
> struct xencons_info *xencons = vtermno_to_xencons(vtermno);
> unsigned int eoiflag = 0;
> + unsigned long flags;
>
> if (xencons == NULL)
> return -EINVAL;
> intf = xencons->intf;
>
> + spin_lock_irqsave(&xencons->ring_lock, flags);
> cons = intf->in_cons;
> prod = intf->in_prod;
> mb(); /* get pointers before reading ring */
>
> if ((prod - cons) > sizeof(intf->in)) {
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> pr_err_once("xencons: Illegal ring page indices");
> return -EINVAL;
> }
> @@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char
> *buf, int len)
> xencons->out_cons = intf->out_cons;
> xencons->out_cons_same = 0;
> }
> + if (!recv && xencons->out_cons_same++ > 1) {
> + eoiflag = XEN_EOI_FLAG_SPURIOUS;
> + }
> + spin_unlock_irqrestore(&xencons->ring_lock, flags);
> +
> if (recv) {
> notify_daemon(xencons);
> - } else if (xencons->out_cons_same++ > 1) {
> - eoiflag = XEN_EOI_FLAG_SPURIOUS;
> }
>
> xen_irq_lateeoi(xencons->irq, eoiflag);
> @@ -576,6 +587,7 @@ static int __init xen_hvc_init(void)
>
> info = vtermno_to_xencons(HVC_COOKIE);
> info->irq = bind_evtchn_to_irq_lateeoi(info->evtchn);
> + spin_lock_init(&info->ring_lock);
Don't we also need a call to spin_lock_init in xencons_connect_backend
and xen_cons_init and xenboot_console_setup ?
> }
> if (info->irq < 0)
> info->irq = 0; /* NO_IRQ */
> --
> 2.37.3
>
Re: [PATCH] xen: replace xen_remap() with memremap()
On Mon, 30 May 2022, Juergen Gross wrote:
> xen_remap() is used to establish mappings for frames not under direct
> control of the kernel: for Xenstore and console ring pages, and for
> grant pages of non-PV guests.
>
> Today xen_remap() is defined to use ioremap() on x86 (doing uncached
> mappings), and ioremap_cache() on Arm (doing cached mappings).
>
> Uncached mappings for those use cases are bad for performance, so they
> should be avoided if possible. As all use cases of xen_remap() don't
> require uncached mappings (the mapped area is always physical RAM),
> a mapping using the standard WB cache mode is fine.
>
> As sparse is flagging some of the xen_remap() use cases to be not
> appropriate for iomem(), as the result is not annotated with the
> __iomem modifier, eliminate xen_remap() completely and replace all
> use cases with memremap() specifying the MEMREMAP_WB caching mode.
>
> xen_unmap() can be replaced with memunmap().
>
> Reported-by: kernel test robot
> Signed-off-by: Juergen Gross
Acked-by: Stefano Stabellini
> ---
> arch/x86/include/asm/xen/page.h | 3 ---
> drivers/tty/hvc/hvc_xen.c | 2 +-
> drivers/xen/grant-table.c | 6 +++---
> drivers/xen/xenbus/xenbus_probe.c | 8
> include/xen/arm/page.h| 3 ---
> 5 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index 1fc67df50014..fa9ec20783fa 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -347,9 +347,6 @@ unsigned long arbitrary_virt_to_mfn(void *vaddr);
> void make_lowmem_page_readonly(void *vaddr);
> void make_lowmem_page_readwrite(void *vaddr);
>
> -#define xen_remap(cookie, size) ioremap((cookie), (size))
> -#define xen_unmap(cookie) iounmap((cookie))
> -
> static inline bool xen_arch_need_swiotlb(struct device *dev,
>phys_addr_t phys,
>dma_addr_t dev_addr)
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index ebaf7500f48f..7c23112dc923 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -253,7 +253,7 @@ static int xen_hvm_console_init(void)
> if (r < 0 || v == 0)
> goto err;
> gfn = v;
> - info->intf = xen_remap(gfn << XEN_PAGE_SHIFT, XEN_PAGE_SIZE);
> + info->intf = memremap(gfn << XEN_PAGE_SHIFT, XEN_PAGE_SIZE,
> MEMREMAP_WB);
> if (info->intf == NULL)
> goto err;
> info->vtermno = HVC_COOKIE;
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 1a1aec0a88a1..2f4f0ed5d8f8 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -632,7 +632,7 @@ int gnttab_setup_auto_xlat_frames(phys_addr_t addr)
> if (xen_auto_xlat_grant_frames.count)
> return -EINVAL;
>
> - vaddr = xen_remap(addr, XEN_PAGE_SIZE * max_nr_gframes);
> + vaddr = memremap(addr, XEN_PAGE_SIZE * max_nr_gframes, MEMREMAP_WB);
> if (vaddr == NULL) {
> pr_warn("Failed to ioremap gnttab share frames (addr=%pa)!\n",
> &addr);
> @@ -640,7 +640,7 @@ int gnttab_setup_auto_xlat_frames(phys_addr_t addr)
> }
> pfn = kcalloc(max_nr_gframes, sizeof(pfn[0]), GFP_KERNEL);
> if (!pfn) {
> - xen_unmap(vaddr);
> + memunmap(vaddr);
> return -ENOMEM;
> }
> for (i = 0; i < max_nr_gframes; i++)
> @@ -659,7 +659,7 @@ void gnttab_free_auto_xlat_frames(void)
> if (!xen_auto_xlat_grant_frames.count)
> return;
> kfree(xen_auto_xlat_grant_frames.pfn);
> - xen_unmap(xen_auto_xlat_grant_frames.vaddr);
> + memunmap(xen_auto_xlat_grant_frames.vaddr);
>
> xen_auto_xlat_grant_frames.pfn = NULL;
> xen_auto_xlat_grant_frames.count = 0;
> diff --git a/drivers/xen/xenbus/xenbus_probe.c
> b/drivers/xen/xenbus/xenbus_probe.c
> index d367f2bd2b93..58b732dcbfb8 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -752,8 +752,8 @@ static void xenbus_probe(void)
> xenstored_ready = 1;
>
> if (!xen_store_interface) {
> - xen_store_interface = xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
> - XEN_PAGE_SIZE);
> + xen_store_interface = memremap(xen_store_gfn << XEN_PAGE_SHIFT,
> +XEN_PAGE_SIZE, MEMREMAP_WB);
> /*
>* Now
Re: [PATCH 13/15] swiotlb: merge swiotlb-xen initialization into swiotlb
On Mon, 14 Mar 2022, Christoph Hellwig wrote: > Reuse the generic swiotlb initialization for xen-swiotlb. For ARM/ARM64 > this works trivially, while for x86 xen_swiotlb_fixup needs to be passed > as the remap argument to swiotlb_init_remap/swiotlb_init_late. > > Signed-off-by: Christoph Hellwig For arch/arm/xen and drivers/xen/swiotlb-xen.c Reviewed-by: Stefano Stabellini
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Fri, 4 Mar 2022, Christoph Hellwig wrote:
> On Thu, Mar 03, 2022 at 02:49:29PM -0800, Stefano Stabellini wrote:
> > On Thu, 3 Mar 2022, Christoph Hellwig wrote:
> > > On Wed, Mar 02, 2022 at 05:25:10PM -0800, Stefano Stabellini wrote:
> > > > Thinking more about it we actually need to drop the xen_initial_domain()
> > > > check otherwise some cases won't be functional (Dom0 not 1:1 mapped, or
> > > > DomU 1:1 mapped).
> > >
> > > Hmm, but that would be the case even before this series, right?
> >
> > Before this series we only have the xen_swiotlb_detect() check in
> > xen_mm_init, we don't have a second xen_initial_domain() check.
> >
> > The issue is that this series is adding one more xen_initial_domain()
> > check in xen_mm_init.
>
> In current mainline xen_mm_init calls xen_swiotlb_init unconditionally.
> But xen_swiotlb_init then calls xen_swiotlb_fixup after allocating
> the memory, which in turn calls xen_create_contiguous_region.
> xen_create_contiguous_region fails with -EINVAL for the
> !xen_initial_domain() and thus caues xen_swiotlb_fixup and
> xen_swiotlb_init to unwind and return -EINVAL.
>
> So as far as I can tell there is no change in behavior, but maybe I'm
> missing something subtle?
You are right.
The xen_initial_domain() check in xen_create_contiguous_region() is
wrong and we should get rid of it. It is a leftover from before the
xen_swiotlb_detect rework.
We could either remove it or change it into another xen_swiotlb_detect()
check.
Feel free to add the patch to your series or fold it with another patch
or rework it as you prefer. Thanks for spotting this!
---
arm/xen: don't check for xen_initial_domain() in xen_create_contiguous_region
It used to be that Linux enabled swiotlb-xen when running a dom0 on ARM.
Since f5079a9a2a31 "xen/arm: introduce XENFEAT_direct_mapped and
XENFEAT_not_direct_mapped", Linux detects whether to enable or disable
swiotlb-xen based on the new feature flags: XENFEAT_direct_mapped and
XENFEAT_not_direct_mapped.
However, there is still a leftover xen_initial_domain() check in
xen_create_contiguous_region. Remove the check as
xen_create_contiguous_region is only called by swiotlb-xen during
initialization. If xen_create_contiguous_region is called, we know Linux
is running 1:1 mapped so there is no need for additional checks.
Also update the in-code comment.
Signed-off-by: Stefano Stabellini
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index a7e54a087b80..28c207060253 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -122,10 +122,7 @@ int xen_create_contiguous_region(phys_addr_t pstart,
unsigned int order,
unsigned int address_bits,
dma_addr_t *dma_handle)
{
- if (!xen_initial_domain())
- return -EINVAL;
-
- /* we assume that dom0 is mapped 1:1 for now */
+ /* the domain is 1:1 mapped to use swiotlb-xen */
*dma_handle = pstart;
return 0;
}
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Thu, 3 Mar 2022, Christoph Hellwig wrote: > On Wed, Mar 02, 2022 at 05:25:10PM -0800, Stefano Stabellini wrote: > > Thinking more about it we actually need to drop the xen_initial_domain() > > check otherwise some cases won't be functional (Dom0 not 1:1 mapped, or > > DomU 1:1 mapped). > > Hmm, but that would be the case even before this series, right? Before this series we only have the xen_swiotlb_detect() check in xen_mm_init, we don't have a second xen_initial_domain() check. The issue is that this series is adding one more xen_initial_domain() check in xen_mm_init.
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Wed, 2 Mar 2022, Christoph Hellwig wrote:
> On Tue, Mar 01, 2022 at 06:55:47PM -0800, Stefano Stabellini wrote:
> > Unrelated to this specific patch series: now that I think about it, if
> > io_tlb_default_mem.nslabs is already allocated by the time xen_mm_init
> > is called, wouldn't we potentially have an issue with the GFP flags used
> > for the earlier allocation (e.g. GFP_DMA32 not used)? Maybe something
> > for another day.
>
> swiotlb_init allocates low memory from meblock, which is roughly
> equivalent to GFP_DMA allocations, so we'll be fine.
>
> > > @@ -143,10 +141,15 @@ static int __init xen_mm_init(void)
> > > if (!xen_swiotlb_detect())
> > > return 0;
> > >
> > > - rc = xen_swiotlb_init();
> > > /* we can work with the default swiotlb */
> > > - if (rc < 0 && rc != -EEXIST)
> > > - return rc;
> > > + if (!io_tlb_default_mem.nslabs) {
> > > + if (!xen_initial_domain())
> > > + return -EINVAL;
> >
> > I don't think we need this xen_initial_domain() check. It is all
> > already sorted out by the xen_swiotlb_detect() check above.
>
> Is it?
>
> static inline int xen_swiotlb_detect(void)
> {
> if (!xen_domain())
> return 0;
> if (xen_feature(XENFEAT_direct_mapped))
> return 1;
> /* legacy case */
> if (!xen_feature(XENFEAT_not_direct_mapped) && xen_initial_domain())
> return 1;
> return 0;
> }
It used to be that we had a
if (!xen_initial_domain())
return -EINVAL;
check in the initialization of swiotlb-xen on ARM. Then we replaced it
with the more sophisticated xen_swiotlb_detect().
The reason is that swiotlb-xen on ARM relies on Dom0 being 1:1 mapped
(guest physical addresses == physical addresses). Recent changes in Xen
allowed also DomUs to be 1:1 mapped. Changes still under discussion will
allow Dom0 not to be 1:1 mapped.
So, before all the Xen-side changes, knowing what was going to happen, I
introduced a clearer interface: XENFEAT_direct_mapped and
XENFEAT_not_direct_mapped tell us whether the guest (Linux) is 1:1
mapped or not. If it is 1:1 mapped then Linux can take advantage of
swiotlb-xen. Now xen_swiotlb_detect() returns true if Linux is 1:1
mapped.
Then of course there is the legacy case. That's taken care of by:
if (!xen_feature(XENFEAT_not_direct_mapped) && xen_initial_domain())
return 1;
The intention is to say that if
XENFEAT_direct_mapped/XENFEAT_not_direct_mapped are not present, then
use xen_initial_domain() like we did before.
So if xen_swiotlb_detect() returns true we know that Linux is either
dom0 (xen_initial_domain() == true) or we have very good reasons to
think we should initialize swiotlb-xen anyway
(xen_feature(XENFEAT_direct_mapped) == true).
> I think I'd keep it as-is for now, as my planned next step would be to
> fold xen-swiotlb into swiotlb entirely.
Thinking more about it we actually need to drop the xen_initial_domain()
check otherwise some cases won't be functional (Dom0 not 1:1 mapped, or
DomU 1:1 mapped).
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Tue, 1 Mar 2022, Christoph Hellwig wrote:
> Allow to pass a remap argument to the swiotlb initialization functions
> to handle the Xen/x86 remap case. ARM/ARM64 never did any remapping
> from xen_swiotlb_fixup, so we don't even need that quirk.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/arm/xen/mm.c | 23 +++---
> arch/x86/include/asm/xen/page.h | 5 --
> arch/x86/kernel/pci-dma.c | 19 +++--
> arch/x86/pci/sta2x11-fixup.c| 2 +-
> drivers/xen/swiotlb-xen.c | 128 +---
> include/linux/swiotlb.h | 7 +-
> include/xen/arm/page.h | 1 -
> include/xen/swiotlb-xen.h | 8 +-
> kernel/dma/swiotlb.c| 120 +++---
> 9 files changed, 96 insertions(+), 217 deletions(-)
>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index a7e54a087b802..58b40f87617d3 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -23,22 +23,20 @@
> #include
> #include
>
> -unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> +static gfp_t xen_swiotlb_gfp(void)
> {
> phys_addr_t base;
> - gfp_t flags = __GFP_NOWARN|__GFP_KSWAPD_RECLAIM;
> u64 i;
>
> for_each_mem_range(i, &base, NULL) {
> if (base < (phys_addr_t)0x) {
> if (IS_ENABLED(CONFIG_ZONE_DMA32))
> - flags |= __GFP_DMA32;
> - else
> - flags |= __GFP_DMA;
> - break;
> + return __GFP_DMA32;
> + return __GFP_DMA;
> }
> }
> - return __get_free_pages(flags, order);
> +
> + return GFP_KERNEL;
> }
Unrelated to this specific patch series: now that I think about it, if
io_tlb_default_mem.nslabs is already allocated by the time xen_mm_init
is called, wouldn't we potentially have an issue with the GFP flags used
for the earlier allocation (e.g. GFP_DMA32 not used)? Maybe something
for another day.
> static bool hypercall_cflush = false;
> @@ -143,10 +141,15 @@ static int __init xen_mm_init(void)
> if (!xen_swiotlb_detect())
> return 0;
>
> - rc = xen_swiotlb_init();
> /* we can work with the default swiotlb */
> - if (rc < 0 && rc != -EEXIST)
> - return rc;
> + if (!io_tlb_default_mem.nslabs) {
> + if (!xen_initial_domain())
> + return -EINVAL;
I don't think we need this xen_initial_domain() check. It is all
already sorted out by the xen_swiotlb_detect() check above.
Aside from that the rest looks OK. Also, you can add my:
Tested-by: Stefano Stabellini
> + rc = swiotlb_init_late(swiotlb_size_or_default(),
> +xen_swiotlb_gfp(), NULL);
> + if (rc < 0)
> + return rc;
> + }
>
> cflush.op = 0;
> cflush.a.dev_bus_addr = 0;
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index e989bc2269f54..1fc67df500145 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -357,9 +357,4 @@ static inline bool xen_arch_need_swiotlb(struct device
> *dev,
> return false;
> }
>
> -static inline unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> -{
> - return __get_free_pages(__GFP_NOWARN, order);
> -}
> -
> #endif /* _ASM_X86_XEN_PAGE_H */
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index e0def4b1c3181..2f2c468acb955 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -71,15 +71,12 @@ static inline void __init pci_swiotlb_detect(void)
> #endif /* CONFIG_SWIOTLB */
>
> #ifdef CONFIG_SWIOTLB_XEN
> -static bool xen_swiotlb;
> -
> static void __init pci_xen_swiotlb_init(void)
> {
> if (!xen_initial_domain() && !x86_swiotlb_enable)
> return;
> x86_swiotlb_enable = false;
> - xen_swiotlb = true;
> - xen_swiotlb_init_early();
> + swiotlb_init_remap(true, x86_swiotlb_flags, xen_swiotlb_fixup);
> dma_ops = &xen_swiotlb_dma_ops;
> if (IS_ENABLED(CONFIG_PCI))
> pci_request_acs();
> @@ -87,14 +84,16 @@ static void __init pci_xen_swiotlb_init(void)
>
> int pci_xen_swiotlb_init_late(void)
> {
> - int rc;
> -
> - if (xen_swiotlb)
> + if (dma_ops == &xen_swiotlb_dma_ops)
> return 0;
>
> - rc = xen_swiotlb_init();
> - if (rc)
> - return rc;
> + /* we can work with the
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On Tue, 14 Sep 2021, Christoph Hellwig wrote: > On Tue, Sep 14, 2021 at 05:29:07PM +0200, Jan Beulich wrote: > > I'm not convinced the swiotlb use describe there falls under "intended > > use" - mapping a 1280x720 framebuffer in a single chunk? (As an aside, > > the bottom of this page is also confusing, as following "Then we can > > confirm the modified swiotlb size in the boot log:" there is a log > > fragment showing the same original size of 64Mb. > > It doesn't. We also do not add hacks to the kernel for whacky out > of tree modules. Also, Option 1 listed in the webpage seems to be a lot better. Any reason you can't do that? Because that option both solves the problem and increases performance.
Re: [PATCH v14 01/12] swiotlb: Refactor swiotlb init functions
On Sat, 19 Jun 2021, Claire Chang wrote:
> Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
> initialization to make the code reusable.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> kernel/dma/swiotlb.c | 50 ++--
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 52e2ac526757..1f9b2b9e7490 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -168,9 +168,28 @@ void __init swiotlb_update_mem_attributes(void)
> memset(vaddr, 0, bytes);
> }
>
> -int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t
> start,
> + unsigned long nslabs, bool late_alloc)
> {
> + void *vaddr = phys_to_virt(start);
> unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> +
> + mem->nslabs = nslabs;
> + mem->start = start;
> + mem->end = mem->start + bytes;
> + mem->index = 0;
> + mem->late_alloc = late_alloc;
> + spin_lock_init(&mem->lock);
> + for (i = 0; i < mem->nslabs; i++) {
> + mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> + mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> + mem->slots[i].alloc_size = 0;
> + }
> + memset(vaddr, 0, bytes);
> +}
> +
> +int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +{
> struct io_tlb_mem *mem;
> size_t alloc_size;
>
> @@ -186,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs, int verbose)
> if (!mem)
> panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> __func__, alloc_size, PAGE_SIZE);
> - mem->nslabs = nslabs;
> - mem->start = __pa(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - spin_lock_init(&mem->lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> +
> + swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
>
> io_tlb_default_mem = mem;
> if (verbose)
> @@ -282,8 +293,8 @@ swiotlb_late_init_with_default_size(size_t default_size)
> int
> swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
> {
> - unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> struct io_tlb_mem *mem;
> + unsigned long bytes = nslabs << IO_TLB_SHIFT;
>
> if (swiotlb_force == SWIOTLB_NO_FORCE)
> return 0;
> @@ -297,20 +308,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> nslabs)
> if (!mem)
> return -ENOMEM;
>
> - mem->nslabs = nslabs;
> - mem->start = virt_to_phys(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - mem->late_alloc = 1;
> - spin_lock_init(&mem->lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> -
> + memset(mem, 0, sizeof(*mem));
> set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT);
> - memset(tlb, 0, bytes);
> + swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true);
>
> io_tlb_default_mem = mem;
> swiotlb_print_info();
> --
> 2.32.0.288.g62a8d224e6-goog
>
Re: [PATCH v13 01/12] swiotlb: Refactor swiotlb init functions
On Fri, 18 Jun 2021, Christoph Hellwig wrote: > On Fri, Jun 18, 2021 at 09:09:17AM -0500, Tom Lendacky wrote: > > > swiotlb_init_with_tbl uses memblock_alloc to allocate the io_tlb_mem > > > and memblock_alloc[1] will do memset in memblock_alloc_try_nid[2], so > > > swiotlb_init_with_tbl is also good. > > > I'm happy to add the memset in swiotlb_init_io_tlb_mem if you think > > > it's clearer and safer. > > > > On x86, if the memset is done before set_memory_decrypted() and memory > > encryption is active, then the memory will look like ciphertext afterwards > > and not be zeroes. If zeroed memory is required, then a memset must be > > done after the set_memory_decrypted() calls. > > Which should be fine - we don't care that the memory is cleared to 0, > just that it doesn't leak other data. Maybe a comment would be useful, > though, Just as a clarification: I was referring to the zeroing of "mem" in swiotlb_late_init_with_tbl and swiotlb_init_with_tbl. While it looks like Tom and Christoph are talking about the zeroing of "tlb". The zeroing of "mem" is required as some fields of struct io_tlb_mem need to be initialized to zero. While the zeroing of "tlb" is not required except from the point of view of not leaking sensitive data as Christoph mentioned. Either way, Claire's v14 patch 01/12 looks fine to me.
Re: [PATCH v13 09/12] swiotlb: Add restricted DMA alloc/free support
On Thu, 17 Jun 2021, Claire Chang wrote:
> Add the functions, swiotlb_{alloc,free} and is_swiotlb_for_alloc to
> support the memory allocation from restricted DMA pool.
>
> The restricted DMA pool is preferred if available.
>
> Note that since coherent allocation needs remapping, one must set up
> another device coherent pool by shared-dma-pool and use
> dma_alloc_from_dev_coherent instead for atomic coherent allocation.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> include/linux/swiotlb.h | 26 ++
> kernel/dma/direct.c | 49 +++--
> kernel/dma/swiotlb.c| 38 ++--
> 3 files changed, 99 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 8d8855c77d9a..a73fad460162 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -85,6 +85,7 @@ extern enum swiotlb_force swiotlb_force;
> * @debugfs: The dentry to debugfs.
> * @late_alloc: %true if allocated using the page allocator
> * @force_bounce: %true if swiotlb bouncing is forced
> + * @for_alloc: %true if the pool is used for memory allocation
> */
> struct io_tlb_mem {
> phys_addr_t start;
> @@ -96,6 +97,7 @@ struct io_tlb_mem {
> struct dentry *debugfs;
> bool late_alloc;
> bool force_bounce;
> + bool for_alloc;
> struct io_tlb_slot {
> phys_addr_t orig_addr;
> size_t alloc_size;
> @@ -156,4 +158,28 @@ static inline void swiotlb_adjust_size(unsigned long
> size)
> extern void swiotlb_print_info(void);
> extern void swiotlb_set_max_segment(unsigned int);
>
> +#ifdef CONFIG_DMA_RESTRICTED_POOL
> +struct page *swiotlb_alloc(struct device *dev, size_t size);
> +bool swiotlb_free(struct device *dev, struct page *page, size_t size);
> +
> +static inline bool is_swiotlb_for_alloc(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->for_alloc;
> +}
> +#else
> +static inline struct page *swiotlb_alloc(struct device *dev, size_t size)
> +{
> + return NULL;
> +}
> +static inline bool swiotlb_free(struct device *dev, struct page *page,
> + size_t size)
> +{
> + return false;
> +}
> +static inline bool is_swiotlb_for_alloc(struct device *dev)
> +{
> + return false;
> +}
> +#endif /* CONFIG_DMA_RESTRICTED_POOL */
> +
> #endif /* __LINUX_SWIOTLB_H */
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index a92465b4eb12..2de33e5d302b 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -75,6 +75,15 @@ static bool dma_coherent_ok(struct device *dev,
> phys_addr_t phys, size_t size)
> min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit);
> }
>
> +static void __dma_direct_free_pages(struct device *dev, struct page *page,
> + size_t size)
> +{
> + if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> + swiotlb_free(dev, page, size))
> + return;
> + dma_free_contiguous(dev, page, size);
> +}
> +
> static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
> gfp_t gfp)
> {
> @@ -86,6 +95,16 @@ static struct page *__dma_direct_alloc_pages(struct device
> *dev, size_t size,
>
> gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
> &phys_limit);
> + if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> + is_swiotlb_for_alloc(dev)) {
> + page = swiotlb_alloc(dev, size);
> + if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> + __dma_direct_free_pages(dev, page, size);
> + return NULL;
> + }
> + return page;
> + }
> +
> page = dma_alloc_contiguous(dev, size, gfp);
> if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> dma_free_contiguous(dev, page, size);
> @@ -142,7 +161,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
> gfp |= __GFP_NOWARN;
>
> if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> - !force_dma_unencrypted(dev)) {
> + !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
> page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO);
> if (!page)
> return NULL;
> @@ -155
Re: [PATCH v13 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Thu, 17 Jun 2021, Claire Chang wrote:
> Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
> use it to determine whether to bounce the data or not. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/xen/swiotlb-xen.c | 2 +-
> include/linux/swiotlb.h | 11 +++
> kernel/dma/direct.c | 2 +-
> kernel/dma/direct.h | 2 +-
> kernel/dma/swiotlb.c | 4
> 5 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 0c6ed09f8513..4730a146fa35 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -369,7 +369,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> if (dma_capable(dev, dev_addr, size, true) &&
> !range_straddles_page_boundary(phys, size) &&
> !xen_arch_need_swiotlb(dev, phys, dev_addr) &&
> - swiotlb_force != SWIOTLB_FORCE)
> + !is_swiotlb_force_bounce(dev))
> goto done;
>
> /*
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index dd1c30a83058..8d8855c77d9a 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -84,6 +84,7 @@ extern enum swiotlb_force swiotlb_force;
> * unmap calls.
> * @debugfs: The dentry to debugfs.
> * @late_alloc: %true if allocated using the page allocator
> + * @force_bounce: %true if swiotlb bouncing is forced
> */
> struct io_tlb_mem {
> phys_addr_t start;
> @@ -94,6 +95,7 @@ struct io_tlb_mem {
> spinlock_t lock;
> struct dentry *debugfs;
> bool late_alloc;
> + bool force_bounce;
> struct io_tlb_slot {
> phys_addr_t orig_addr;
> size_t alloc_size;
> @@ -109,6 +111,11 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> return mem && paddr >= mem->start && paddr < mem->end;
> }
>
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->force_bounce;
> +}
> +
> void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> @@ -120,6 +127,10 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> {
> return false;
> }
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return false;
> +}
> static inline void swiotlb_exit(void)
> {
> }
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 7a88c34d0867..a92465b4eb12 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev)
> {
> /* If SWIOTLB is active, use its maximum mapping size */
> if (is_swiotlb_active(dev) &&
> - (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
> + (dma_addressing_limited(dev) || is_swiotlb_force_bounce(dev)))
> return swiotlb_max_mapping_size(dev);
> return SIZE_MAX;
> }
> diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h
> index 13e9e7158d94..4632b0f4f72e 100644
> --- a/kernel/dma/direct.h
> +++ b/kernel/dma/direct.h
> @@ -87,7 +87,7 @@ static inline dma_addr_t dma_direct_map_page(struct device
> *dev,
> phys_addr_t phys = page_to_phys(page) + offset;
> dma_addr_t dma_addr = phys_to_dma(dev, phys);
>
> - if (unlikely(swiotlb_force == SWIOTLB_FORCE))
> + if (is_swiotlb_force_bounce(dev))
> return swiotlb_map(dev, phys, size, dir, attrs);
>
> if (unlikely(!dma_capable(dev, dma_addr, size, true))) {
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 409694d7a8ad..13891d5de8c9 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -179,6 +179,10 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem
> *mem, phys_addr_t start,
> mem->end = mem->start + bytes;
> mem->index = 0;
> mem->late_alloc = late_alloc;
> +
> + if (swiotlb_force == SWIOTLB_FORCE)
> + mem->force_bounce = true;
> +
> spin_lock_init(&mem->lock);
> for (i = 0; i < mem->nslabs; i++) {
> mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> --
> 2.32.0.288.g62a8d224e6-goog
>
Re: [PATCH v13 05/12] swiotlb: Update is_swiotlb_active to add a struct device argument
On Thu, 17 Jun 2021, Claire Chang wrote:
> Update is_swiotlb_active to add a struct device argument. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_ttm.c| 2 +-
> drivers/pci/xen-pcifront.c | 2 +-
> include/linux/swiotlb.h | 4 ++--
> kernel/dma/direct.c | 2 +-
> kernel/dma/swiotlb.c | 4 ++--
> 6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index a9d65fc8aa0e..4b7afa0fc85d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -42,7 +42,7 @@ static int i915_gem_object_get_pages_internal(struct
> drm_i915_gem_object *obj)
>
> max_order = MAX_ORDER;
> #ifdef CONFIG_SWIOTLB
> - if (is_swiotlb_active()) {
> + if (is_swiotlb_active(obj->base.dev->dev)) {
> unsigned int max_segment;
>
> max_segment = swiotlb_max_segment();
> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> index 9662522aa066..be15bfd9e0ee 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> @@ -321,7 +321,7 @@ nouveau_ttm_init(struct nouveau_drm *drm)
> }
>
> #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)
> - need_swiotlb = is_swiotlb_active();
> + need_swiotlb = is_swiotlb_active(dev->dev);
> #endif
>
> ret = ttm_bo_device_init(&drm->ttm.bdev, &nouveau_bo_driver,
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index b7a8f3a1921f..0d56985bfe81 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -693,7 +693,7 @@ static int pcifront_connect_and_init_dma(struct
> pcifront_device *pdev)
>
> spin_unlock(&pcifront_dev_lock);
>
> - if (!err && !is_swiotlb_active()) {
> + if (!err && !is_swiotlb_active(&pdev->xdev->dev)) {
> err = pci_xen_swiotlb_init_late();
> if (err)
> dev_err(&pdev->xdev->dev, "Could not setup SWIOTLB!\n");
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index d1f3d95881cd..dd1c30a83058 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -112,7 +112,7 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> -bool is_swiotlb_active(void);
> +bool is_swiotlb_active(struct device *dev);
> void __init swiotlb_adjust_size(unsigned long size);
> #else
> #define swiotlb_force SWIOTLB_NO_FORCE
> @@ -132,7 +132,7 @@ static inline size_t swiotlb_max_mapping_size(struct
> device *dev)
> return SIZE_MAX;
> }
>
> -static inline bool is_swiotlb_active(void)
> +static inline bool is_swiotlb_active(struct device *dev)
> {
> return false;
> }
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 84c9feb5474a..7a88c34d0867 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -495,7 +495,7 @@ int dma_direct_supported(struct device *dev, u64 mask)
> size_t dma_direct_max_mapping_size(struct device *dev)
> {
> /* If SWIOTLB is active, use its maximum mapping size */
> - if (is_swiotlb_active() &&
> + if (is_swiotlb_active(dev) &&
> (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
> return swiotlb_max_mapping_size(dev);
> return SIZE_MAX;
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index de79e9437030..409694d7a8ad 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -664,9 +664,9 @@ size_t swiotlb_max_mapping_size(struct device *dev)
> return ((size_t)IO_TLB_SIZE) * IO_TLB_SEGSIZE;
> }
>
> -bool is_swiotlb_active(void)
> +bool is_swiotlb_active(struct device *dev)
> {
> - return io_tlb_default_mem != NULL;
> + return dev->dma_io_tlb_mem != NULL;
> }
> EXPORT_SYMBOL_GPL(is_swiotlb_active);
>
> --
> 2.32.0.288.g62a8d224e6-goog
>
Re: [PATCH v13 04/12] swiotlb: Update is_swiotlb_buffer to add a struct device argument
On Thu, 17 Jun 2021, Claire Chang wrote:
> Update is_swiotlb_buffer to add a struct device argument. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/iommu/dma-iommu.c | 12 ++--
> drivers/xen/swiotlb-xen.c | 2 +-
> include/linux/swiotlb.h | 7 ---
> kernel/dma/direct.c | 6 +++---
> kernel/dma/direct.h | 6 +++---
> 5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 3087d9fa6065..10997ef541f8 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -507,7 +507,7 @@ static void __iommu_dma_unmap_swiotlb(struct device *dev,
> dma_addr_t dma_addr,
>
> __iommu_dma_unmap(dev, dma_addr, size);
>
> - if (unlikely(is_swiotlb_buffer(phys)))
> + if (unlikely(is_swiotlb_buffer(dev, phys)))
> swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
> }
>
> @@ -578,7 +578,7 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device
> *dev, phys_addr_t phys,
> }
>
> iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
> - if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
> + if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(dev, phys))
> swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
> return iova;
> }
> @@ -749,7 +749,7 @@ static void iommu_dma_sync_single_for_cpu(struct device
> *dev,
> if (!dev_is_dma_coherent(dev))
> arch_sync_dma_for_cpu(phys, size, dir);
>
> - if (is_swiotlb_buffer(phys))
> + if (is_swiotlb_buffer(dev, phys))
> swiotlb_sync_single_for_cpu(dev, phys, size, dir);
> }
>
> @@ -762,7 +762,7 @@ static void iommu_dma_sync_single_for_device(struct
> device *dev,
> return;
>
> phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
> - if (is_swiotlb_buffer(phys))
> + if (is_swiotlb_buffer(dev, phys))
> swiotlb_sync_single_for_device(dev, phys, size, dir);
>
> if (!dev_is_dma_coherent(dev))
> @@ -783,7 +783,7 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
> if (!dev_is_dma_coherent(dev))
> arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir);
>
> - if (is_swiotlb_buffer(sg_phys(sg)))
> + if (is_swiotlb_buffer(dev, sg_phys(sg)))
> swiotlb_sync_single_for_cpu(dev, sg_phys(sg),
> sg->length, dir);
> }
> @@ -800,7 +800,7 @@ static void iommu_dma_sync_sg_for_device(struct device
> *dev,
> return;
>
> for_each_sg(sgl, sg, nelems, i) {
> - if (is_swiotlb_buffer(sg_phys(sg)))
> + if (is_swiotlb_buffer(dev, sg_phys(sg)))
> swiotlb_sync_single_for_device(dev, sg_phys(sg),
> sg->length, dir);
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 4c89afc0df62..0c6ed09f8513 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -100,7 +100,7 @@ static int is_xen_swiotlb_buffer(struct device *dev,
> dma_addr_t dma_addr)
>* in our domain. Therefore _only_ check address within our domain.
>*/
> if (pfn_valid(PFN_DOWN(paddr)))
> - return is_swiotlb_buffer(paddr);
> + return is_swiotlb_buffer(dev, paddr);
> return 0;
> }
>
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 216854a5e513..d1f3d95881cd 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -2,6 +2,7 @@
> #ifndef __LINUX_SWIOTLB_H
> #define __LINUX_SWIOTLB_H
>
> +#include
> #include
> #include
> #include
> @@ -101,9 +102,9 @@ struct io_tlb_mem {
> };
> extern struct io_tlb_mem *io_tlb_default_mem;
>
> -static inline bool is_swiotlb_buffer(phys_addr_t paddr)
> +static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
>
> return mem && paddr >= mem->start && paddr < mem->end;
> }
> @@ -115,7 +116,7 @@ bool is_swiotlb_active(void);
> void __init swiotlb_adjust_size(unsigned long size);
> #else
> #define swiotlb_force SWIOTLB_NO_FORCE
> -static
Re: [PATCH v13 03/12] swiotlb: Set dev->dma_io_tlb_mem to the swiotlb pool used
On Thu, 17 Jun 2021, Claire Chang wrote:
> Always have the pointer to the swiotlb pool used in struct device. This
> could help simplify the code for other pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/base/core.c| 4
> include/linux/device.h | 4
> kernel/dma/swiotlb.c | 8
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f29839382f81..cb3123e3954d 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include /* for dma_default_coherent */
>
> @@ -2736,6 +2737,9 @@ void device_initialize(struct device *dev)
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> dev->dma_coherent = dma_default_coherent;
> #endif
> +#ifdef CONFIG_SWIOTLB
> + dev->dma_io_tlb_mem = io_tlb_default_mem;
> +#endif
> }
> EXPORT_SYMBOL_GPL(device_initialize);
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ba660731bd25..240d652a0696 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -416,6 +416,7 @@ struct dev_links_info {
> * @dma_pools: Dma pools (if dma'ble device).
> * @dma_mem: Internal for coherent mem override.
> * @cma_area:Contiguous memory area for dma allocations
> + * @dma_io_tlb_mem: Pointer to the swiotlb pool used. Not for driver use.
> * @archdata:For arch-specific additions.
> * @of_node: Associated device tree node.
> * @fwnode: Associated device node supplied by platform firmware.
> @@ -518,6 +519,9 @@ struct device {
> #ifdef CONFIG_DMA_CMA
> struct cma *cma_area; /* contiguous memory area for dma
> allocations */
> +#endif
> +#ifdef CONFIG_SWIOTLB
> + struct io_tlb_mem *dma_io_tlb_mem;
> #endif
> /* arch specific additions */
> struct dev_archdata archdata;
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 2dba659a1e73..de79e9437030 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -340,7 +340,7 @@ void __init swiotlb_exit(void)
> static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t
> size,
> enum dma_data_direction dir)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
> unsigned int offset = (tlb_addr - mem->start) & (IO_TLB_SIZE - 1);
> phys_addr_t orig_addr = mem->slots[index].orig_addr;
> @@ -431,7 +431,7 @@ static unsigned int wrap_index(struct io_tlb_mem *mem,
> unsigned int index)
> static int find_slots(struct device *dev, phys_addr_t orig_addr,
> size_t alloc_size)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> unsigned long boundary_mask = dma_get_seg_boundary(dev);
> dma_addr_t tbl_dma_addr =
> phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> @@ -508,7 +508,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev,
> phys_addr_t orig_addr,
> size_t mapping_size, size_t alloc_size,
> enum dma_data_direction dir, unsigned long attrs)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> unsigned int i;
> int index;
> @@ -559,7 +559,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev,
> phys_addr_t tlb_addr,
> size_t mapping_size, enum dma_data_direction dir,
> unsigned long attrs)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = hwdev->dma_io_tlb_mem;
> unsigned long flags;
> unsigned int offset = swiotlb_align_offset(hwdev, tlb_addr);
> int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT;
> --
> 2.32.0.288.g62a8d224e6-goog
>
Re: [PATCH v13 01/12] swiotlb: Refactor swiotlb init functions
On Thu, 17 Jun 2021, Claire Chang wrote:
> Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
> initialization to make the code reusable.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
> ---
> kernel/dma/swiotlb.c | 50 ++--
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 52e2ac526757..47bb2a766798 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -168,9 +168,28 @@ void __init swiotlb_update_mem_attributes(void)
> memset(vaddr, 0, bytes);
> }
>
> -int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t
> start,
> + unsigned long nslabs, bool late_alloc)
> {
> + void *vaddr = phys_to_virt(start);
> unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> +
> + mem->nslabs = nslabs;
> + mem->start = start;
> + mem->end = mem->start + bytes;
> + mem->index = 0;
> + mem->late_alloc = late_alloc;
> + spin_lock_init(&mem->lock);
> + for (i = 0; i < mem->nslabs; i++) {
> + mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> + mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> + mem->slots[i].alloc_size = 0;
> + }
> + memset(vaddr, 0, bytes);
> +}
> +
> +int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +{
> struct io_tlb_mem *mem;
> size_t alloc_size;
>
> @@ -186,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs, int verbose)
> if (!mem)
> panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> __func__, alloc_size, PAGE_SIZE);
> - mem->nslabs = nslabs;
> - mem->start = __pa(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - spin_lock_init(&mem->lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> +
> + swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
>
> io_tlb_default_mem = mem;
> if (verbose)
> @@ -282,8 +293,8 @@ swiotlb_late_init_with_default_size(size_t default_size)
> int
> swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
> {
> - unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> struct io_tlb_mem *mem;
> + unsigned long bytes = nslabs << IO_TLB_SHIFT;
>
> if (swiotlb_force == SWIOTLB_NO_FORCE)
> return 0;
> @@ -297,20 +308,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> nslabs)
> if (!mem)
> return -ENOMEM;
>
> - mem->nslabs = nslabs;
> - mem->start = virt_to_phys(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - mem->late_alloc = 1;
> - spin_lock_init(&mem->lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> -
> + memset(mem, 0, sizeof(*mem));
> + swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true);
> set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT);
> - memset(tlb, 0, bytes);
This is good for swiotlb_late_init_with_tbl. However I have just noticed
that mem could also be allocated from swiotlb_init_with_tbl, in which
case the zeroing is missing. I think we need another memset in
swiotlb_init_with_tbl as well. Or maybe it could be better to have a
single memset at the beginning of swiotlb_init_io_tlb_mem instead. Up to
you.
Re: [PATCH v12 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Wed, 16 Jun 2021, Claire Chang wrote:
> Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
> use it to determine whether to bounce the data or not. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> ---
> include/linux/swiotlb.h | 11 +++
> kernel/dma/direct.c | 2 +-
> kernel/dma/direct.h | 2 +-
> kernel/dma/swiotlb.c| 4
> 4 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index dd1c30a83058..8d8855c77d9a 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -84,6 +84,7 @@ extern enum swiotlb_force swiotlb_force;
> * unmap calls.
> * @debugfs: The dentry to debugfs.
> * @late_alloc: %true if allocated using the page allocator
> + * @force_bounce: %true if swiotlb bouncing is forced
> */
> struct io_tlb_mem {
> phys_addr_t start;
> @@ -94,6 +95,7 @@ struct io_tlb_mem {
> spinlock_t lock;
> struct dentry *debugfs;
> bool late_alloc;
> + bool force_bounce;
> struct io_tlb_slot {
> phys_addr_t orig_addr;
> size_t alloc_size;
> @@ -109,6 +111,11 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> return mem && paddr >= mem->start && paddr < mem->end;
> }
>
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->force_bounce;
> +}
> void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> @@ -120,6 +127,10 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> {
> return false;
> }
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return false;
> +}
> static inline void swiotlb_exit(void)
> {
> }
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 7a88c34d0867..a92465b4eb12 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev)
> {
> /* If SWIOTLB is active, use its maximum mapping size */
> if (is_swiotlb_active(dev) &&
> - (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
> + (dma_addressing_limited(dev) || is_swiotlb_force_bounce(dev)))
> return swiotlb_max_mapping_size(dev);
> return SIZE_MAX;
> }
> diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h
> index 13e9e7158d94..4632b0f4f72e 100644
> --- a/kernel/dma/direct.h
> +++ b/kernel/dma/direct.h
> @@ -87,7 +87,7 @@ static inline dma_addr_t dma_direct_map_page(struct device
> *dev,
> phys_addr_t phys = page_to_phys(page) + offset;
> dma_addr_t dma_addr = phys_to_dma(dev, phys);
>
> - if (unlikely(swiotlb_force == SWIOTLB_FORCE))
> + if (is_swiotlb_force_bounce(dev))
> return swiotlb_map(dev, phys, size, dir, attrs);
>
> if (unlikely(!dma_capable(dev, dma_addr, size, true))) {
Should we also make the same change in
drivers/xen/swiotlb-xen.c:xen_swiotlb_map_page ?
If I make that change, I can see that everything is working as
expected for a restricted-dma device with Linux running as dom0 on Xen.
However, is_swiotlb_force_bounce returns non-zero even for normal
non-restricted-dma devices. That shouldn't happen, right?
It looks like struct io_tlb_slot is not zeroed on allocation.
Adding memset(mem, 0x0, struct_size) in swiotlb_late_init_with_tbl
solves the issue.
With those two changes, the series passes my tests and you can add my
tested-by.
Re: [PATCH v12 11/12] dt-bindings: of: Add restricted DMA pool
On Wed, 16 Jun 2021, Claire Chang wrote:
> Introduce the new compatible string, restricted-dma-pool, for restricted
> DMA. One can specify the address and length of the restricted DMA memory
> region by restricted-dma-pool in the reserved-memory node.
>
> Signed-off-by: Claire Chang
> ---
> .../reserved-memory/reserved-memory.txt | 36 +--
> 1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index e8d3096d922c..46804f24df05 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -51,6 +51,23 @@ compatible (optional) - standard definition
>used as a shared pool of DMA buffers for a set of devices. It can
>be used by an operating system to instantiate the necessary pool
>management subsystem if necessary.
> +- restricted-dma-pool: This indicates a region of memory meant to be
> + used as a pool of restricted DMA buffers for a set of devices. The
> + memory region would be the only region accessible to those devices.
> + When using this, the no-map and reusable properties must not be
> set,
> + so the operating system can create a virtual mapping that will be
> used
> + for synchronization. The main purpose for restricted DMA is to
> + mitigate the lack of DMA access control on systems without an
> IOMMU,
> + which could result in the DMA accessing the system memory at
> + unexpected times and/or unexpected addresses, possibly leading to
> data
> + leakage or corruption. The feature on its own provides a basic
> level
> + of protection against the DMA overwriting buffer contents at
> + unexpected times. However, to protect against general data leakage
> and
> + system memory corruption, the system needs to provide way to lock
> down
> + the memory access, e.g., MPU. Note that since coherent allocation
> + needs remapping, one must set up another device coherent pool by
> + shared-dma-pool and use dma_alloc_from_dev_coherent instead for
> atomic
> + coherent allocation.
> - vendor specific string in the form ,[-]
> no-map (optional) - empty property
> - Indicates the operating system must not create a virtual mapping
> @@ -85,10 +102,11 @@ memory-region-names (optional) - a list of names, one
> for each corresponding
>
> Example
> ---
> -This example defines 3 contiguous regions are defined for Linux kernel:
> +This example defines 4 contiguous regions for Linux kernel:
> one default of all device drivers (named linux,cma@7200 and 64MiB in
> size),
> -one dedicated to the framebuffer device (named framebuffer@7800, 8MiB),
> and
> -one for multimedia processing (named multimedia-memory@7700, 64MiB).
> +one dedicated to the framebuffer device (named framebuffer@7800, 8MiB),
> +one for multimedia processing (named multimedia-memory@7700, 64MiB), and
> +one for restricted dma pool (named restricted_dma_reserved@0x5000,
> 64MiB).
>
> / {
> #address-cells = <1>;
> @@ -120,6 +138,11 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> compatible = "acme,multimedia-memory";
> reg = <0x7700 0x400>;
> };
> +
> + restricted_dma_reserved: restricted_dma_reserved {
> + compatible = "restricted-dma-pool";
> + reg = <0x5000 0x400>;
> + };
> };
>
> /* ... */
> @@ -138,4 +161,11 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> memory-region = <&multimedia_reserved>;
> /* ... */
> };
> +
> + pcie_device: pcie_device@0,0 {
> + reg = <0x8301 0x0 0x 0x0 0x0010
> +0x8301 0x0 0x0010 0x0 0x0010>;
> + memory-region = <&restricted_dma_mem_reserved>;
Shouldn't it be &restricted_dma_reserved ?
Re: [PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays
On Fri, 19 Feb 2021, Konrad Rzeszutek Wilk wrote:
> On Sun, Feb 07, 2021 at 04:56:01PM +0100, Christoph Hellwig wrote:
> > On Thu, Feb 04, 2021 at 09:40:23AM +0100, Christoph Hellwig wrote:
> > > So one thing that has been on my mind for a while: I'd really like
> > > to kill the separate dma ops in Xen swiotlb. If we compare xen-swiotlb
> > > to swiotlb the main difference seems to be:
> > >
> > > - additional reasons to bounce I/O vs the plain DMA capable
> > > - the possibility to do a hypercall on arm/arm64
> > > - an extra translation layer before doing the phys_to_dma and vice
> > >versa
> > > - an special memory allocator
> > >
> > > I wonder if inbetween a few jump labels or other no overhead enablement
> > > options and possibly better use of the dma_range_map we could kill
> > > off most of swiotlb-xen instead of maintaining all this code duplication?
> >
> > So I looked at this a bit more.
> >
> > For x86 with XENFEAT_auto_translated_physmap (how common is that?)
>
> Juergen, Boris please correct me if I am wrong, but that
> XENFEAT_auto_translated_physmap
> only works for PVH guests?
ARM is always XENFEAT_auto_translated_physmap
> > pfn_to_gfn is a nop, so plain phys_to_dma/dma_to_phys do work as-is.
> >
> > xen_arch_need_swiotlb always returns true for x86, and
> > range_straddles_page_boundary should never be true for the
> > XENFEAT_auto_translated_physmap case.
>
> Correct. The kernel should have no clue of what the real MFNs are
> for PFNs.
On ARM, Linux knows the MFNs because for local pages MFN == PFN and for
foreign pages it keeps track in arch/arm/xen/p2m.c. More on this below.
xen_arch_need_swiotlb only returns true on ARM in rare situations where
bouncing on swiotlb buffers is required. Today it only happens on old
versions of Xen that don't support the cache flushing hypercall but
there could be more cases in the future.
> >
> > So as far as I can tell the mapping fast path for the
> > XENFEAT_auto_translated_physmap can be trivially reused from swiotlb.
> >
> > That leaves us with the next more complicated case, x86 or fully cache
> > coherent arm{,64} without XENFEAT_auto_translated_physmap. In that case
> > we need to patch in a phys_to_dma/dma_to_phys that performs the MFN
> > lookup, which could be done using alternatives or jump labels.
> > I think if that is done right we should also be able to let that cover
> > the foreign pages in is_xen_swiotlb_buffer/is_swiotlb_buffer, but
> > in that worst case that would need another alternative / jump label.
> >
> > For non-coherent arm{,64} we'd also need to use alternatives or jump
> > labels to for the cache maintainance ops, but that isn't a hard problem
> > either.
With the caveat that ARM is always XENFEAT_auto_translated_physmap, what
you wrote looks correct. I am writing down a brief explanation on how
swiotlb-xen is used on ARM.
pfn: address as seen by the guest, pseudo-physical address in ARM terminology
mfn (or bfn): real address, physical address in ARM terminology
On ARM dom0 is auto_translated (so Xen sets up the stage2 translation
in the MMU) and the translation is 1:1. So pfn == mfn for Dom0.
However, when another domain shares a page with Dom0, that page is not
1:1. Swiotlb-xen is used to retrieve the mfn for the foreign page at
xen_swiotlb_map_page. It does that with xen_phys_to_bus -> pfn_to_bfn.
It is implemented with a rbtree in arch/arm/xen/p2m.c.
In addition, swiotlb-xen is also used to cache-flush the page via
hypercall at xen_swiotlb_unmap_page. That is done because dev_addr is
really the mfn at unmap_page and we don't know the pfn for it. We can do
pfn-to-mfn but we cannot do mfn-to-pfn (there are good reasons for it
unfortunately). The only way to cache-flush by mfn is by issuing a
hypercall. The hypercall is implemented in arch/arm/xen/mm.c.
The pfn != bfn and pfn_valid() checks are used to detect if the page is
local (of dom0) or foreign; they work thanks to the fact that Dom0 is
1:1 mapped.
Getting back to what you wrote, yes if we had a way to do MFN lookups in
phys_to_dma, and a way to call the hypercall at unmap_page if the page
is foreign (e.g. if it fails a pfn_valid check) then I think we would be
good from an ARM perspective. The only exception is when
xen_arch_need_swiotlb returns true, in which case we need to actually
bounce on swiotlb buffers.
Re: next build: 37 warnings 2 failures (next/next-20160519)
On Thu, 19 May 2016, Arnd Bergmann wrote: > > 2 drivers/xen/balloon.c:154:13: warning: 'release_memory_resource' > > declared 'static' but never defined [-Wunused-function] > > I sent a patch on May 11, subject "xen: remove incorrect forward declaration" > and > Stefano Stabellini reviewed it. Ross Lagerwall did the same patch a day > earlier, > but neither of them has made it into linux-next so far. According to Ross, > this > one should be backported to v4.4. It's on our radar, the patch hasn't been lost. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3] swiotlb: prefix dma_to_phys and phys_to_dma functions
On Mon, 28 Mar 2016, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 18, 2016 at 11:00 AM, Sinan Kaya wrote:
> > On 3/18/2016 8:12 AM, Robin Murphy wrote:
> >> Since we know for sure that swiotlb_to_phys is a no-op on arm64, it might
> >> be cleaner to simply not reference it at all. I suppose we could have some
> >> private local wrappers, e.g.:
> >>
> >> #define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
> >>
> >> to keep the intent of the code clear (and just in case anyone ever builds
> >> a system mad enough to warrant switching out that definition, but I'd hope
> >> that never happens).
> >>
> >> Otherwise, looks good - thanks for doing this!
> >
> > OK. I added this. Reviewed-by?
> >
> > I'm not happy to submit such a big patch for all different ARCHs. I couldn't
> > find a cleaner solution. I'm willing to split this patch into multiple if
> > there
> > is a better way.
> >
> > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> > index ada00c3..8c0f66b 100644
> > --- a/arch/arm64/mm/dma-mapping.c
> > +++ b/arch/arm64/mm/dma-mapping.c
> > @@ -29,6 +29,14 @@
> >
> > #include
> >
> > +/*
> > + * If you are building a system without IOMMU, then you are using SWIOTLB
> > + * library. The ARM64 adaptation of this library does not support address
> > + * translation and it assumes that physical address = dma address for such
> > + * a use case. Please don't build a platform that violates this.
> > + */
>
> Why not just expand the ARM64 part to support address translation?
>
> > +#define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
> > +
>
> Adding Stefano here.
Could you please explain what is the problem that you are trying to
solve? In other words, what is the issue with assuming that physical
address = dma address (and the current dma_to_phys and phys_to_dma
static inlines) if no arm64 platforms violate it? That's pretty much
what is done on x86 too (without X86_DMA_REMAP).
If you want to make sure that the assumption is not violated, you can
introduce a boot time check or a BUG_ON somewhere.
If there is an arm64 platform with phys_addr != dma_addr, we need proper
support for it. In fact even if there is an IOMMU on that platform, when
running Xen on it, the IOMMU would be used by the hypervisor and Linux
would still end up without it, using the swiotlb.
> > static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
> > bool coherent)
> > {
> > @@ -188,7 +196,7 @@ static void __dma_free(struct device *dev, size_t size,
> >void *vaddr, dma_addr_t dma_handle,
> >struct dma_attrs *attrs)
> > {
> > - void *swiotlb_addr = phys_to_virt(swiotlb_dma_to_phys(dev,
> > dma_handle));
> > + void *swiotlb_addr = swiotlb_to_virt(dma_handle);
> >
> > size = PAGE_ALIGN(size);
> >
> > @@ -209,8 +217,7 @@ static dma_addr_t __swiotlb_map_page(struct device
> > *dev, struct page *page,
> >
> > dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
> > if (!is_device_dma_coherent(dev))
> > - __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev,
> > dev_addr)),
> > - size, dir);
> > + __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
> >
> > return dev_addr;
> > }
> > @@ -283,8 +290,7 @@ static void __swiotlb_sync_single_for_device(struct
> > device *dev,
> > {
> > swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
> > if (!is_device_dma_coherent(dev))
> > - __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev,
> > dev_addr)),
> > - size, dir);
> > + __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
> > }
> >
> >
> >
> >
> > --
> > Sinan Kaya
> > Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> > Foundation Collaborative Project
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] tty: hvc_xen: hide xen_console_remove when unused
On Mon, 25 Jan 2016, Boris Ostrovsky wrote:
> On 01/25/2016 04:54 PM, Arnd Bergmann wrote:
> > xencons_disconnect_backend() is only called from xen_console_remove(),
>
> and also from xencons_probe()/xencons_resume(). But those two are also under
> the
> same ifdef.
Good point. Aside from this the patch is good.
> > which is conditionally compiled, so we get a harmless warning when
> > CONFIG_HVC_XEN_FRONTEND is unset:
> >
> > hvc/hvc_xen.c:350:12: error: 'xen_console_remove' defined but not used
> > [-Werror=unused-function]
> >
> > This moves the function down into the same #ifdef section to silence
> > the warning.
> >
> > Signed-off-by: Arnd Bergmann
> > ---
> > drivers/tty/hvc/hvc_xen.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index fa816b7193b6..11725422dacb 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -323,6 +323,7 @@ void xen_console_resume(void)
> > }
> > }
> > +#ifdef CONFIG_HVC_XEN_FRONTEND
> > static void xencons_disconnect_backend(struct xencons_info *info)
> > {
> > if (info->irq > 0)
> > @@ -363,7 +364,6 @@ static int xen_console_remove(struct xencons_info *info)
> > return 0;
> > }
> > -#ifdef CONFIG_HVC_XEN_FRONTEND
> > static int xencons_remove(struct xenbus_device *dev)
> > {
> > return xen_console_remove(dev_get_drvdata(&dev->dev));
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 33/34] xenbus: use virt_xxx barriers
On Thu, 31 Dec 2015, Michael S. Tsirkin wrote:
> drivers/xen/xenbus/xenbus_comms.c uses
> full memory barriers to communicate with the other side.
>
> For guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> would be sufficient, so mb() and wmb() here are only needed if
> a non-SMP guest runs on an SMP host.
>
> Switch to virt_xxx barriers which serve this exact purpose.
>
> Signed-off-by: Michael S. Tsirkin
Reviewed-by: Stefano Stabellini
Are you also going to take care of
drivers/xen/grant-table.c
drivers/xen/evtchn.c
drivers/xen/events/events_fifo.c
drivers/xen/xen-scsiback.c
drivers/xen/tmem.c
drivers/xen/xen-pciback/pci_stub.c
drivers/xen/xen-pciback/pciback_ops.c
?
> drivers/xen/xenbus/xenbus_comms.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_comms.c
> b/drivers/xen/xenbus/xenbus_comms.c
> index fdb0f33..ecdecce 100644
> --- a/drivers/xen/xenbus/xenbus_comms.c
> +++ b/drivers/xen/xenbus/xenbus_comms.c
> @@ -123,14 +123,14 @@ int xb_write(const void *data, unsigned len)
> avail = len;
>
> /* Must write data /after/ reading the consumer index. */
> - mb();
> + virt_mb();
>
> memcpy(dst, data, avail);
> data += avail;
> len -= avail;
>
> /* Other side must not see new producer until data is there. */
> - wmb();
> + virt_wmb();
> intf->req_prod += avail;
>
> /* Implies mb(): other side will see the updated producer. */
> @@ -180,14 +180,14 @@ int xb_read(void *data, unsigned len)
> avail = len;
>
> /* Must read data /after/ reading the producer index. */
> - rmb();
> + virt_rmb();
>
> memcpy(data, src, avail);
> data += avail;
> len -= avail;
>
> /* Other side must not see free space until we've copied out */
> - mb();
> + virt_mb();
> intf->rsp_cons += avail;
>
> pr_debug("Finished read of %i bytes (%i to go)\n", avail, len);
> --
> MST
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 34/34] xen/io: use virt_xxx barriers
On Thu, 31 Dec 2015, Michael S. Tsirkin wrote:
> include/xen/interface/io/ring.h uses
> full memory barriers to communicate with the other side.
>
> For guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> would be sufficient, so mb() and wmb() here are only needed if
> a non-SMP guest runs on an SMP host.
>
> Switch to virt_xxx barriers which serve this exact purpose.
>
> Signed-off-by: Michael S. Tsirkin
Reviewed-by: Stefano Stabellini
> include/xen/interface/io/ring.h | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 7dc685b..21f4fbd 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -208,12 +208,12 @@ struct __name##_back_ring {
> \
>
>
> #define RING_PUSH_REQUESTS(_r) do { \
> -wmb(); /* back sees requests /before/ updated producer index */ \
> +virt_wmb(); /* back sees requests /before/ updated producer index */
> \
> (_r)->sring->req_prod = (_r)->req_prod_pvt;
> \
> } while (0)
>
> #define RING_PUSH_RESPONSES(_r) do { \
> -wmb(); /* front sees responses /before/ updated producer index */
> \
> +virt_wmb(); /* front sees responses /before/ updated producer index */
> \
> (_r)->sring->rsp_prod = (_r)->rsp_prod_pvt;
> \
> } while (0)
>
> @@ -250,9 +250,9 @@ struct __name##_back_ring {
> \
> #define RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(_r, _notify) do {
> \
> RING_IDX __old = (_r)->sring->req_prod; \
> RING_IDX __new = (_r)->req_prod_pvt; \
> -wmb(); /* back sees requests /before/ updated producer index */ \
> +virt_wmb(); /* back sees requests /before/ updated producer index */
> \
> (_r)->sring->req_prod = __new; \
> -mb(); /* back sees new requests /before/ we check req_event */ \
> +virt_mb(); /* back sees new requests /before/ we check req_event */
> \
> (_notify) = ((RING_IDX)(__new - (_r)->sring->req_event) <
> \
>(RING_IDX)(__new - __old));\
> } while (0)
> @@ -260,9 +260,9 @@ struct __name##_back_ring {
> \
> #define RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(_r, _notify) do {
> \
> RING_IDX __old = (_r)->sring->rsp_prod; \
> RING_IDX __new = (_r)->rsp_prod_pvt; \
> -wmb(); /* front sees responses /before/ updated producer index */
> \
> +virt_wmb(); /* front sees responses /before/ updated producer index */
> \
> (_r)->sring->rsp_prod = __new; \
> -mb(); /* front sees new responses /before/ we check rsp_event */ \
> +virt_mb(); /* front sees new responses /before/ we check rsp_event */
> \
> (_notify) = ((RING_IDX)(__new - (_r)->sring->rsp_event) <
> \
>(RING_IDX)(__new - __old));\
> } while (0)
> @@ -271,7 +271,7 @@ struct __name##_back_ring {
> \
> (_work_to_do) = RING_HAS_UNCONSUMED_REQUESTS(_r);
> \
> if (_work_to_do) break; \
> (_r)->sring->req_event = (_r)->req_cons + 1; \
> -mb();\
> +virt_mb();
> \
> (_work_to_do) = RING_HAS_UNCONSUMED_REQUESTS(_r);
> \
> } while (0)
>
> @@ -279,7 +279,7 @@ struct __name##_back_ring {
> \
> (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);
> \
> if (_work_to_do) break; \
> (_r)->sring->rsp_event = (_r)->rsp_cons + 1; \
> -mb();\
> +virt_mb();
> \
> (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);
> \
> } while (0)
>
> --
> MST
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Xen-devel] [PATCH v2 4/8] xen: Use the correctly the Xen memory terminologies
On Thu, 6 Aug 2015, Julien Grall wrote:
> On 06/08/15 12:06, Stefano Stabellini wrote:
> > On Thu, 6 Aug 2015, Julien Grall wrote:
> >> Hi,
> >>
> >>
> >> On 04/08/15 19:12, Julien Grall wrote:
> >>> diff --git a/include/xen/page.h b/include/xen/page.h
> >>> index c5ed20b..e7e1425 100644
> >>> --- a/include/xen/page.h
> >>> +++ b/include/xen/page.h
> >>> @@ -3,9 +3,9 @@
> >>>
> >>> #include
> >>>
> >>> -static inline unsigned long page_to_mfn(struct page *page)
> >>> +static inline unsigned long page_to_gfn(struct page *page)
> >>> {
> >>> - return pfn_to_mfn(page_to_pfn(page));
> >>> + return pfn_to_gfn(page_to_pfn(page));
> >>> }
> >>
> >> I've just noticed that there is a function gfn_to_page used for KVM.
> >>
> >> Maybe I should rename page_to_gfn to xen_page_to_gfn to avoid confusion
> >> with KVM one?
> >
> > Yeah, prepending xen would help to avoid namespace pollution.
>
> Will do. May I keep your Reviewed-by for this mechanical change?
Yes
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Xen-devel] [PATCH v2 4/8] xen: Use the correctly the Xen memory terminologies
On Thu, 6 Aug 2015, Julien Grall wrote:
> Hi,
>
>
> On 04/08/15 19:12, Julien Grall wrote:
> > diff --git a/include/xen/page.h b/include/xen/page.h
> > index c5ed20b..e7e1425 100644
> > --- a/include/xen/page.h
> > +++ b/include/xen/page.h
> > @@ -3,9 +3,9 @@
> >
> > #include
> >
> > -static inline unsigned long page_to_mfn(struct page *page)
> > +static inline unsigned long page_to_gfn(struct page *page)
> > {
> > - return pfn_to_mfn(page_to_pfn(page));
> > + return pfn_to_gfn(page_to_pfn(page));
> > }
>
> I've just noticed that there is a function gfn_to_page used for KVM.
>
> Maybe I should rename page_to_gfn to xen_page_to_gfn to avoid confusion
> with KVM one?
Yeah, prepending xen would help to avoid namespace pollution.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 4/8] xen: Use the correctly the Xen memory terminologies
On Tue, 4 Aug 2015, Julien Grall wrote:
> Based on include/xen/mm.h [1], Linux is mistakenly using MFN when GFN
> is meant, I suspect this is because the first support for Xen was for
> PV. This resulted in some misimplementation of helpers on ARM and
> confused developers about the expected behavior.
>
> For instance, with pfn_to_mfn, we expect to get an MFN based on the name.
> Although, if we look at the implementation on x86, it's returning a GFN.
>
> For clarity and avoid new confusion, replace any reference to mfn with
> gfn in any helpers used by PV drivers. The x86 code will still keep some
> reference of pfn_to_mfn but exclusively for PV (a BUG_ON has been added
> to ensure this). No changes as been made in the hypercall field, even
> though they may be invalid, in order to keep the same as the defintion
> in xen repo.
>
> Take also the opportunity to simplify simple construction such
> as pfn_to_mfn(page_to_pfn(page)) into page_to_gfn. More complex clean up
> will come in follow-up patches.
>
> [1]
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=e758ed14f390342513405dd766e874934573e6cb
>
> Signed-off-by: Julien Grall
> Cc: Stefano Stabellini
> Cc: Russell King
> Cc: Konrad Rzeszutek Wilk
> Cc: Boris Ostrovsky
> Cc: David Vrabel
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: "H. Peter Anvin"
> Cc: x...@kernel.org
> Cc: "Roger Pau Monné"
> Cc: Dmitry Torokhov
> Cc: Ian Campbell
> Cc: Wei Liu
> Cc: Juergen Gross
> Cc: "James E.J. Bottomley"
> Cc: Greg Kroah-Hartman
> Cc: Jiri Slaby
> Cc: Jean-Christophe Plagniol-Villard
> Cc: Tomi Valkeinen
> Cc: linux-in...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Cc: linux-s...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-fb...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
Aside from the x86 bits:
Reviewed-by: Stefano Stabellini
> Note that I've re-introduced mfn_to_pfn & co only for x86 PV code.
> The helpers contain a BUG_ON to ensure that it's never called for
> auto-translated guests. I did as best as my can to determine whether
> mfn or gfn helpers should be used. Although, I haven't tried to boot
> it.
>
> It may be possible to do further cleanup in the mmu.c where I found
> some check to auto-translated. I'm not sure why given that the pvmmu
> callback are only used for non-auto translated guest.
>
> Finally, given those changes, I didn't retain the Reviewed-by/Acked-by.
>
> Changes in v2:
> - Give directly the URL to the commit rather than the commit ID
> - xenstored_local_init: keep the cast to void *
> - Typoes
> - Keep pfn_to_mfn for x86 and PV-only. The *mfn* helpers are
> used in arch/x86/xen for enlighten.c, mmu.c, p2m.c, setup.c,
> smp.c and mm.c
> ---
> arch/arm/include/asm/xen/page.h | 13 +++--
> arch/x86/include/asm/xen/page.h | 33
> ++---
> arch/x86/xen/smp.c | 2 +-
> drivers/block/xen-blkfront.c| 6 +++---
> drivers/input/misc/xen-kbdfront.c | 4 ++--
> drivers/net/xen-netback/netback.c | 4 ++--
> drivers/net/xen-netfront.c | 8
> drivers/scsi/xen-scsifront.c| 8 +++-
> drivers/tty/hvc/hvc_xen.c | 5 +++--
> drivers/video/fbdev/xen-fbfront.c | 4 ++--
> drivers/xen/balloon.c | 2 +-
> drivers/xen/events/events_base.c| 2 +-
> drivers/xen/events/events_fifo.c| 4 ++--
> drivers/xen/gntalloc.c | 3 ++-
> drivers/xen/manage.c| 2 +-
> drivers/xen/tmem.c | 4 ++--
> drivers/xen/xenbus/xenbus_client.c | 2 +-
> drivers/xen/xenbus/xenbus_dev_backend.c | 2 +-
> drivers/xen/xenbus/xenbus_probe.c | 8 +++-
> include/xen/page.h | 4 ++--
> 20 files changed, 69 insertions(+), 51 deletions(-)
>
> diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
> index 087d86e..51e5bf1 100644
> --- a/arch/arm/include/asm/xen/page.h
> +++ b/arch/arm/include/asm/xen/page.h
> @@ -34,14 +34,15 @@ typedef struct xpaddr {
> unsigned long __pfn_to_mfn(unsigned long pfn);
> extern struct rb_root phys_to_mach;
>
> -static inline unsigned long pfn_to_mfn(unsigned long pfn)
> +/* Pseudo-physical <-> Guest conversion */
> +static inline unsigned long pfn_to_gfn(unsigned long pfn)
> {
> return pfn;
> }
>
> -static inline unsigned long mfn_to_pfn(unsigned long mfn)
> +static inline unsign
Re: [PATCH v2 4/8] xen: Use the correctly the Xen memory terminologies
On Tue, 4 Aug 2015, Boris Ostrovsky wrote:
> On 08/04/2015 02:12 PM, Julien Grall wrote:
> > /*
> >* We detect special mappings in one of two ways:
> > @@ -217,9 +232,13 @@ static inline unsigned long bfn_to_local_pfn(unsigned
> > long mfn)
> > /* VIRT <-> MACHINE conversion */
> > #define virt_to_machine(v)(phys_to_machine(XPADDR(__pa(v
> > -#define virt_to_pfn(v) (PFN_DOWN(__pa(v)))
> > #define virt_to_mfn(v)(pfn_to_mfn(virt_to_pfn(v)))
> > #define mfn_to_virt(m)(__va(mfn_to_pfn(m) << PAGE_SHIFT))
> > +#define virt_to_pfn(v) (PFN_DOWN(__pa(v)))
>
> This looks like unnecessary change.
>
>
> > diff --git a/drivers/video/fbdev/xen-fbfront.c
> > b/drivers/video/fbdev/xen-fbfront.c
> > index 09dc447..25e3cce 100644
> > --- a/drivers/video/fbdev/xen-fbfront.c
> > +++ b/drivers/video/fbdev/xen-fbfront.c
> > @@ -539,7 +539,7 @@ static int xenfb_remove(struct xenbus_device *dev)
> > static unsigned long vmalloc_to_mfn(void *address)
> > {
> > - return pfn_to_mfn(vmalloc_to_pfn(address));
> > + return pfn_to_gfn(vmalloc_to_pfn(address));
> > }
>
> Are you sure? This will return vmalloc_to_pfn(address)).
I think that is OK: there is no behavioural change here.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/8] xen: Use the correctly the Xen memory terminologies
On Tue, 28 Jul 2015, Julien Grall wrote:
> Based on include/xen/mm.h [1], Linux is mistakenly using MFN when GFN
> is meant, I suspect this is because the first support for Xen was for
> PV. This brough some misimplementation of helpers on ARM and make the
> developper confused the expected behavior.
>
> For instance, with pfn_to_mfn, we expect to get an MFN based on the name.
> Although, if we look at the implementation on x86, it's returning a GFN.
>
> For clarity and avoid new confusion, replace any reference of mfn into
> gnf in any helpers used by PV drivers.
>
> Take also the opportunity to simplify simple construction such
> as pfn_to_mfn(page_to_pfn(page)) into page_to_gfn. More complex clean up
> will come in follow-up patches.
>
> I think it may be possible to do further clean up in the x86 code to
> ensure that helpers returning machine address (such as virt_address) is
> not used by no auto-translated guests. I will let x86 xen expert doing
> it.
>
> [1] Xen tree: e758ed14f390342513405dd766e874934573e6cb
>
> Signed-off-by: Julien Grall
Aside from the many typos
Reviewed-by: Stefano Stabellini
> arch/arm/include/asm/xen/page.h | 13 +++--
> arch/x86/include/asm/xen/page.h | 30 --
> arch/x86/xen/enlighten.c| 4 ++--
> arch/x86/xen/mmu.c | 16
> arch/x86/xen/p2m.c | 32
> arch/x86/xen/setup.c| 12 ++--
> arch/x86/xen/smp.c | 4 ++--
> arch/x86/xen/suspend.c | 8
> drivers/block/xen-blkfront.c| 6 +++---
> drivers/input/misc/xen-kbdfront.c | 4 ++--
> drivers/net/xen-netback/netback.c | 4 ++--
> drivers/net/xen-netfront.c | 8
> drivers/scsi/xen-scsifront.c| 8 +++-
> drivers/tty/hvc/hvc_xen.c | 5 +++--
> drivers/video/fbdev/xen-fbfront.c | 4 ++--
> drivers/xen/balloon.c | 2 +-
> drivers/xen/events/events_base.c| 2 +-
> drivers/xen/events/events_fifo.c| 4 ++--
> drivers/xen/gntalloc.c | 3 ++-
> drivers/xen/manage.c| 2 +-
> drivers/xen/tmem.c | 4 ++--
> drivers/xen/xenbus/xenbus_client.c | 2 +-
> drivers/xen/xenbus/xenbus_dev_backend.c | 2 +-
> drivers/xen/xenbus/xenbus_probe.c | 8 +++-
> include/xen/page.h | 4 ++--
> 25 files changed, 96 insertions(+), 95 deletions(-)
>
> diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
> index 493471f..f542f68 100644
> --- a/arch/arm/include/asm/xen/page.h
> +++ b/arch/arm/include/asm/xen/page.h
> @@ -34,14 +34,15 @@ typedef struct xpaddr {
> unsigned long __pfn_to_mfn(unsigned long pfn);
> extern struct rb_root phys_to_mach;
>
> -static inline unsigned long pfn_to_mfn(unsigned long pfn)
> +/* Pseudo-physical <-> Guest conversion */
> +static inline unsigned long pfn_to_gfn(unsigned long pfn)
> {
> return pfn;
> }
>
> -static inline unsigned long mfn_to_pfn(unsigned long mfn)
> +static inline unsigned long gfn_to_pfn(unsigned long gfn)
> {
> - return mfn;
> + return gfn;
> }
>
> /* Pseudo-physical <-> DMA conversion */
> @@ -65,9 +66,9 @@ static inline unsigned long dfn_to_pfn(unsigned long dfn)
>
> #define dfn_to_local_pfn(dfn)dfn_to_pfn(dfn)
>
> -/* VIRT <-> MACHINE conversion */
> -#define virt_to_mfn(v) (pfn_to_mfn(virt_to_pfn(v)))
> -#define mfn_to_virt(m) (__va(mfn_to_pfn(m) << PAGE_SHIFT))
> +/* VIRT <-> GUEST conversion */
> +#define virt_to_gfn(v) (pfn_to_gfn(virt_to_pfn(v)))
> +#define gfn_to_virt(m) (__va(gfn_to_pfn(m) << PAGE_SHIFT))
>
> /* Only used in PV code. However ARM guest is always assimilated as HVM. */
> static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index 046e91a..72d9f15 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -99,7 +99,7 @@ static inline unsigned long __pfn_to_mfn(unsigned long pfn)
> return mfn;
> }
>
> -static inline unsigned long pfn_to_mfn(unsigned long pfn)
> +static inline unsigned long pfn_to_gfn(unsigned long pfn)
> {
> unsigned long mfn;
>
> @@ -145,23 +145,23 @@ static inline unsigned long
> mfn_to_pfn_no_overrides(unsigned long mfn)
> return pfn;
> }
>
> -static inline unsigned long mfn_to_p
Re: [PATCH v2 11/20] tty/hvc: xen: Use xen page definition
On Thu, 9 Jul 2015, Julien Grall wrote: > The console ring is always based on the page granularity of Xen. > > Signed-off-by: Julien Grall > Cc: Greg Kroah-Hartman > Cc: Jiri Slaby > Cc: David Vrabel > Cc: Stefano Stabellini > Cc: Boris Ostrovsky > Cc: linuxppc-dev@lists.ozlabs.org Reviewed-by: Stefano Stabellini > drivers/tty/hvc/hvc_xen.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > index a9d837f..2135944 100644 > --- a/drivers/tty/hvc/hvc_xen.c > +++ b/drivers/tty/hvc/hvc_xen.c > @@ -230,7 +230,7 @@ static int xen_hvm_console_init(void) > if (r < 0 || v == 0) > goto err; > mfn = v; > - info->intf = xen_remap(mfn << PAGE_SHIFT, PAGE_SIZE); > + info->intf = xen_remap(mfn << XEN_PAGE_SHIFT, XEN_PAGE_SIZE); > if (info->intf == NULL) > goto err; > info->vtermno = HVC_COOKIE; > @@ -392,7 +392,7 @@ static int xencons_connect_backend(struct xenbus_device > *dev, > if (xen_pv_domain()) > mfn = virt_to_mfn(info->intf); > else > - mfn = __pa(info->intf) >> PAGE_SHIFT; > + mfn = __pa(info->intf) >> XEN_PAGE_SHIFT; > ret = gnttab_alloc_grant_references(1, &gref_head); > if (ret < 0) > return ret; > @@ -476,7 +476,7 @@ static int xencons_resume(struct xenbus_device *dev) > struct xencons_info *info = dev_get_drvdata(&dev->dev); > > xencons_disconnect_backend(info); > - memset(info->intf, 0, PAGE_SIZE); > + memset(info->intf, 0, XEN_PAGE_SIZE); > return xencons_connect_backend(dev, info); > } > > -- > 2.1.4 > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [patch] hvc_xen: NULL dereference on allocation failure
On Tue, 15 May 2012, Dan Carpenter wrote: > If kzalloc() returns a NULL here, we pass a NULL to > xencons_disconnect_backend() which will cause an Oops. > > Also I removed the __GFP_ZERO while I was at it since kzalloc() implies > __GFP_ZERO. > > Signed-off-by: Dan Carpenter Acked-by: Stefano Stabellini ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
