- Linuxarm - mailweb.openeuler.org

[PATCH 3/9] tty: hvc_console: Remove trailing whitespace
by Xiaofei Tan 17 May '21

17 May '21

Remove trailing whitespace, reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index ddf07ff..13f63d5 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -204,7 +204,7 @@ static struct tty_driver *hvc_console_device(struct console *c, int *index) } static int hvc_console_setup(struct console *co, char *options) -{ +{ if (co->index < 0 || co->index >= MAX_NR_HVC_CONSOLES) return -ENODEV; -- 2.8.1

1 0

[PATCH 2/9] tty: hvc_console: Fix "foo * bar" should be "foo *bar"
by Xiaofei Tan 17 May '21

17 May '21

Fix "foo * bar" should be "foo *bar", reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index a1eca9d..ddf07ff 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -344,7 +344,7 @@ static int hvc_install(struct tty_driver *driver, struct tty_struct *tty) * The TTY interface won't be used until after the vio layer has exposed the vty * adapter to the kernel. */ -static int hvc_open(struct tty_struct *tty, struct file * filp) +static int hvc_open(struct tty_struct *tty, struct file *filp) { struct hvc_struct *hp = tty->driver_data; unsigned long flags; @@ -386,7 +386,7 @@ static int hvc_open(struct tty_struct *tty, struct file * filp) return rc; } -static void hvc_close(struct tty_struct *tty, struct file * filp) +static void hvc_close(struct tty_struct *tty, struct file *filp) { struct hvc_struct *hp = tty->driver_data; unsigned long flags; -- 2.8.1

1 0

[PATCH 1/9] tty: hvc_console: Fix spaces required around that '='
by Xiaofei Tan 17 May '21

17 May '21

Fix spaces required around that '=', reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index d0f0253..a1eca9d 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -944,7 +944,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, * find index to use: * see if this vterm id matches one registered for console. */ - for (i=0; i < MAX_NR_HVC_CONSOLES; i++) + for (i = 0; i < MAX_NR_HVC_CONSOLES; i++) if (vtermnos[i] == hp->vtermno && cons_ops[i] == hp->ops) break; -- 2.8.1

1 0

Re: [PATCH net v8 1/3] net: sched: fix packet stuck problem for lockless qdisc
by Cong Wang 15 May '21

15 May '21

On Thu, May 13, 2021 at 7:27 PM Yunsheng Lin <linyunsheng(a)huawei.com> wrote: > struct qdisc_size_table { > @@ -159,8 +160,33 @@ static inline bool qdisc_is_empty(const struct Qdisc *qdisc) > static inline bool qdisc_run_begin(struct Qdisc *qdisc) > { > if (qdisc->flags & TCQ_F_NOLOCK) { > + if (spin_trylock(&qdisc->seqlock)) > + goto nolock_empty; > + > + /* If the MISSED flag is set, it means other thread has > + * set the MISSED flag before second spin_trylock(), so > + * we can return false here to avoid multi cpus doing > + * the set_bit() and second spin_trylock() concurrently. > + */ > + if (test_bit(__QDISC_STATE_MISSED, &qdisc->state)) > + return false; > + > + /* Set the MISSED flag before the second spin_trylock(), > + * if the second spin_trylock() return false, it means > + * other cpu holding the lock will do dequeuing for us > + * or it will see the MISSED flag set after releasing > + * lock and reschedule the net_tx_action() to do the > + * dequeuing. > + */ > + set_bit(__QDISC_STATE_MISSED, &qdisc->state); > + > + /* Retry again in case other CPU may not see the new flag > + * after it releases the lock at the end of qdisc_run_end(). > + */ > if (!spin_trylock(&qdisc->seqlock)) > return false; > + > +nolock_empty: > WRITE_ONCE(qdisc->empty, false); > } else if (qdisc_is_running(qdisc)) { > return false; > @@ -176,8 +202,15 @@ static inline bool qdisc_run_begin(struct Qdisc *qdisc) > static inline void qdisc_run_end(struct Qdisc *qdisc) > { > write_seqcount_end(&qdisc->running); > - if (qdisc->flags & TCQ_F_NOLOCK) > + if (qdisc->flags & TCQ_F_NOLOCK) { > spin_unlock(&qdisc->seqlock); > + > + if (unlikely(test_bit(__QDISC_STATE_MISSED, > + &qdisc->state))) { > + clear_bit(__QDISC_STATE_MISSED, &qdisc->state); We have test_and_clear_bit() which is atomic, test_bit()+clear_bit() is not. > + __netif_schedule(qdisc); > + } > + } > } > > static inline bool qdisc_may_bulk(const struct Qdisc *qdisc) > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 44991ea..795d986 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -640,8 +640,10 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc) > { > struct pfifo_fast_priv *priv = qdisc_priv(qdisc); > struct sk_buff *skb = NULL; > + bool need_retry = true; > int band; > > +retry: > for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) { > struct skb_array *q = band2list(priv, band); > > @@ -652,6 +654,23 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc) > } > if (likely(skb)) { > qdisc_update_stats_at_dequeue(qdisc, skb); > + } else if (need_retry && > + test_bit(__QDISC_STATE_MISSED, &qdisc->state)) { > + /* Delay clearing the STATE_MISSED here to reduce > + * the overhead of the second spin_trylock() in > + * qdisc_run_begin() and __netif_schedule() calling > + * in qdisc_run_end(). > + */ > + clear_bit(__QDISC_STATE_MISSED, &qdisc->state); Ditto. > + > + /* Make sure dequeuing happens after clearing > + * STATE_MISSED. > + */ > + smp_mb__after_atomic(); > + > + need_retry = false; > + > + goto retry; Two concurrent pfifo_fast_dequeue() would possibly retry it at the same time when they test __QDISC_STATE_MISSED at the same time and get true. Is this a problem? Also, any reason why you want pfifo_fast to handle a generic Qdisc flag? IOW, why not handle this logic in, for example, qdisc_restart()? Thanks.

2 3

Re: [PATCH net v8 0/3] fix packet stuck problem for lockless qdisc
by patchwork-bot+netdevbpf＠kernel.org 15 May '21

15 May '21

Hello: This series was applied to netdev/net.git (refs/heads/master): On Fri, 14 May 2021 11:16:58 +0800 you wrote: > This patchset fixes the packet stuck problem mentioned in [1]. > > Patch 1: Add STATE_MISSED flag to fix packet stuck problem. > Patch 2: Fix a tx_action rescheduling problem after STATE_MISSED > flag is added in patch 1. > Patch 3: Fix the significantly higher CPU consumption problem when > multiple threads are competing on a saturated outgoing > device. > > [...] Here is the summary with links: - [net,v8,1/3] net: sched: fix packet stuck problem for lockless qdisc https://git.kernel.org/netdev/net/c/a90c57f2cedd - [net,v8,2/3] net: sched: fix tx action rescheduling issue during deactivation https://git.kernel.org/netdev/net/c/102b55ee92f9 - [net,v8,3/3] net: sched: fix tx action reschedule issue with stopped queue https://git.kernel.org/netdev/net/c/dcad9ee9e066 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html

1 0

Re: [PATCH net v8 0/3] fix packet stuck problem for lockless qdisc
by Yunsheng Lin 14 May '21

14 May '21

On 2021/5/14 10:26, Yunsheng Lin wrote: > This patchset fixes the packet stuck problem mentioned in [1]. > > Patch 1: Add STATE_MISSED flag to fix packet stuck problem. > Patch 2: Fix a tx_action rescheduling problem after STATE_MISSED > flag is added in patch 1. > Patch 3: Fix the significantly higher CPU consumption problem when > multiple threads are competing on a saturated outgoing > device. > > V8: Change function name in patch 3 as suggested by Jakub, adjust > commit log in patch 2, and add Acked-by from Jakub. Please ignore this patchset, there is some typo in patch 3. > V7: Fix netif_tx_wake_queue() data race noted by Jakub. > V6: Some performance optimization in patch 1 suggested by Jakub > and drop NET_XMIT_DROP checking in patch 3. > V5: add patch 3 to fix the problem reported by Michal Kubecek. > V4: Change STATE_NEED_RESCHEDULE to STATE_MISSED and add patch 2. > > [1]. https://lkml.org/lkml/2019/10/9/42 > > Yunsheng Lin (3): > net: sched: fix packet stuck problem for lockless qdisc > net: sched: fix tx action rescheduling issue during deactivation > net: sched: fix tx action reschedule issue with stopped queue > > include/net/pkt_sched.h | 7 +------ > include/net/sch_generic.h | 35 ++++++++++++++++++++++++++++++++- > net/core/dev.c | 29 ++++++++++++++++++++++----- > net/sched/sch_generic.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 107 insertions(+), 14 deletions(-) >

1 0

Re: [RFC PATCH 2/3] vfio/hisilicon: register the driver to vfio
by Alex Williamson 14 May '21

14 May '21

On Thu, 13 May 2021 17:52:56 +0000 Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> wrote: > Hi Alex, > > > -----Original Message----- > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: 13 May 2021 18:04 > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> > > Cc: Jason Gunthorpe <jgg(a)nvidia.com>; liulongfang > > <liulongfang(a)huawei.com>; cohuck(a)redhat.com; > > linux-kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > > Subject: [Linuxarm] Re: [RFC PATCH 2/3] vfio/hisilicon: register the driver to > > vfio > > > > On Thu, 13 May 2021 15:49:25 +0000 > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> > > wrote: > > > > > > -----Original Message----- > > > > From: Jason Gunthorpe [mailto:jgg@nvidia.com] > > > > Sent: 13 May 2021 14:44 > > > > To: liulongfang <liulongfang(a)huawei.com> > > > > Cc: Alex Williamson <alex.williamson(a)redhat.com>; cohuck(a)redhat.com; > > > > linux-kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > > > > Subject: [Linuxarm] Re: [RFC PATCH 2/3] vfio/hisilicon: register the driver to > > > > vfio > > > > > > > > On Thu, May 13, 2021 at 10:08:28AM +0800, liulongfang wrote: > > > > > On 2021/5/12 20:10, Jason Gunthorpe wrote: > > > > > > On Wed, May 12, 2021 at 04:39:43PM +0800, liulongfang wrote: > > > > > > > > > > > >> Therefore, this method of limiting the length of the BAR > > > > > >> configuration space can prevent unsafe operations of the memory. > > > > > > > > > > > > The issue is DMA controlled by the guest accessing the secure BAR > > > > > > area, not the guest CPU. > > > > > > > > > > > > Jason > > > > > > . > > > > > > > > > > > This secure BAR area is not presented to the Guest, > > > > > which makes it impossible for the Guest to obtain the secure BAR area > > > > > when establishing the DMA mapping of the configuration space. > > > > > If the DMA controller accesses the secure BAR area, the access will > > > > > be blocked by the SMMU. > > > > > > > > There are scenarios where this is not true. > > > > > > > > At a minimum the mdev driver should refuse to work in those cases. > > > > > > > > > > Hi, > > > > > > I think the idea here is not a generic solution, but a quirk for this specific dev. > > > > > > Something like, > > > > > > --- a/drivers/vfio/pci/vfio_pci.c > > > +++ b/drivers/vfio/pci/vfio_pci.c > > > @@ -866,7 +866,12 @@ static long vfio_pci_ioctl(struct vfio_device > > *core_vdev, > > > break; > > > case VFIO_PCI_BAR0_REGION_INDEX ... > > VFIO_PCI_BAR5_REGION_INDEX: > > > info.offset = > > VFIO_PCI_INDEX_TO_OFFSET(info.index); > > > - info.size = pci_resource_len(pdev, info.index); > > > + > > > + if (check_hisi_acc_quirk(pdev, info)) > > > + info.size = new_size;// BAR is limited > > without migration region. > > > + else > > > + info.size = pci_resource_len(pdev, > > info.index); > > > + > > > if (!info.size) { > > > info.flags = 0; > > > break; > > > > > > Is this an acceptable/workable solution here? > > > > As Jason says, this only restricts CPU access to the BAR, the issue is > > DMA access. As the hardware vendor you may be able to guarantee that > > a DMA transaction generated by the device targeting the remainder of > > the BAR will always go upstream, but can you guarantee the routing > > between the device and the SMMU? For instance if this device can be > > implemented as a plugin card, then it can be installed into a > > downstream port that may not support ACS. That downstream port may > > implement request redirection allowing the transaction to reflect back > > to the device without IOMMU translation. At that point the userspace > > driver can target the kernel driver half of the BAR and potentially > > expose a security risk. Thanks, > > The ACC devices on this platform are not pluggable devices. They are exposed > as integrated endpoint devices. So I am not sure the above concern is valid in this > case. > > I had a look at the userspace driver approach you suggested. But unfortunately > the migration state change for the vf has to check some of the pf registers for > confirming the state. So even if we move the implementation to Qemu, we > still may have to use the migration uAPI to access the pf device registers. > > Since the devices we are concerned here are all integrated endpoints and if the > above quirk is an acceptable one, then we can use the uAPI as done in this > series without overly complicating things here. If you expect this device to appear only as an integrated endpoint, then I think Jason's suggestion above is correct. Your driver that supports migration can refuse to load for devices there the topology is other than expected and you're effectively guaranteeing DMA isolation of the user and in-kernel drivers by hardware DMA semantics and topology. Requiring access to the PF to support the migration protocol also suggests that an in-kernel driver to support migration is our best option. Thanks, Alex

3 2

Re: [RFC PATCH 2/3] vfio/hisilicon: register the driver to vfio
by Alex Williamson 14 May '21

14 May '21

On Thu, 13 May 2021 15:49:25 +0000 Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> wrote: > > -----Original Message----- > > From: Jason Gunthorpe [mailto:jgg@nvidia.com] > > Sent: 13 May 2021 14:44 > > To: liulongfang <liulongfang(a)huawei.com> > > Cc: Alex Williamson <alex.williamson(a)redhat.com>; cohuck(a)redhat.com; > > linux-kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > > Subject: [Linuxarm] Re: [RFC PATCH 2/3] vfio/hisilicon: register the driver to > > vfio > > > > On Thu, May 13, 2021 at 10:08:28AM +0800, liulongfang wrote: > > > On 2021/5/12 20:10, Jason Gunthorpe wrote: > > > > On Wed, May 12, 2021 at 04:39:43PM +0800, liulongfang wrote: > > > > > > > >> Therefore, this method of limiting the length of the BAR > > > >> configuration space can prevent unsafe operations of the memory. > > > > > > > > The issue is DMA controlled by the guest accessing the secure BAR > > > > area, not the guest CPU. > > > > > > > > Jason > > > > . > > > > > > > This secure BAR area is not presented to the Guest, > > > which makes it impossible for the Guest to obtain the secure BAR area > > > when establishing the DMA mapping of the configuration space. > > > If the DMA controller accesses the secure BAR area, the access will > > > be blocked by the SMMU. > > > > There are scenarios where this is not true. > > > > At a minimum the mdev driver should refuse to work in those cases. > > > > Hi, > > I think the idea here is not a generic solution, but a quirk for this specific dev. > > Something like, > > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -866,7 +866,12 @@ static long vfio_pci_ioctl(struct vfio_device *core_vdev, > break; > case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX: > info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index); > - info.size = pci_resource_len(pdev, info.index); > + > + if (check_hisi_acc_quirk(pdev, info)) > + info.size = new_size;// BAR is limited without migration region. > + else > + info.size = pci_resource_len(pdev, info.index); > + > if (!info.size) { > info.flags = 0; > break; > > Is this an acceptable/workable solution here? As Jason says, this only restricts CPU access to the BAR, the issue is DMA access. As the hardware vendor you may be able to guarantee that a DMA transaction generated by the device targeting the remainder of the BAR will always go upstream, but can you guarantee the routing between the device and the SMMU? For instance if this device can be implemented as a plugin card, then it can be installed into a downstream port that may not support ACS. That downstream port may implement request redirection allowing the transaction to reflect back to the device without IOMMU translation. At that point the userspace driver can target the kernel driver half of the BAR and potentially expose a security risk. Thanks, Alex

1 0

[PATCH net v7 0/3] fix packet stuck problem for lockless qdisc
by Yunsheng Lin 13 May '21

13 May '21

This patchset fixes the packet stuck problem mentioned in [1]. Patch 1: Add STATE_MISSED flag to fix packet stuck problem. Patch 2: Fix a tx_action rescheduling problem after STATE_MISSED flag is added in patch 1. Patch 3: Fix the significantly higher CPU consumption problem when multiple threads are competing on a saturated outgoing device. V7: Fix netif_tx_wake_queue() data race noted by Jakub. V6: Some performance optimization in patch 1 suggested by Jakub and drop NET_XMIT_DROP checking in patch 3. V5: add patch 3 to fix the problem reported by Michal Kubecek. V4: Change STATE_NEED_RESCHEDULE to STATE_MISSED and add patch 2. [1]. https://lkml.org/lkml/2019/10/9/42 Yunsheng Lin (3): net: sched: fix packet stuck problem for lockless qdisc net: sched: fix endless tx action reschedule during deactivation net: sched: fix tx action reschedule issue with stopped queue include/net/pkt_sched.h | 7 +------ include/net/sch_generic.h | 35 ++++++++++++++++++++++++++++++++- net/core/dev.c | 29 ++++++++++++++++++++++----- net/sched/sch_generic.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 107 insertions(+), 14 deletions(-) -- 2.7.4

2 5

Re: [RFC PATCH 2/3] vfio/hisilicon: register the driver to vfio
by Jason Gunthorpe 13 May '21

13 May '21

On Thu, May 13, 2021 at 10:08:28AM +0800, liulongfang wrote: > On 2021/5/12 20:10, Jason Gunthorpe wrote: > > On Wed, May 12, 2021 at 04:39:43PM +0800, liulongfang wrote: > > > >> Therefore, this method of limiting the length of the BAR > >> configuration space can prevent unsafe operations of the memory. > > > > The issue is DMA controlled by the guest accessing the secure BAR > > area, not the guest CPU. > > > > Jason > > . > > > This secure BAR area is not presented to the Guest, > which makes it impossible for the Guest to obtain the secure BAR area > when establishing the DMA mapping of the configuration space. > If the DMA controller accesses the secure BAR area, the access will > be blocked by the SMMU. There are scenarios where this is not true. At a minimum the mdev driver should refuse to work in those cases. Jason

1 0