thr3ads.net - Virtualization - [PATCH RFC 2/5] s390: add pci_iomap

If this information is useful, please help other people find it:
Share via:

Sebastian Ott

2014-Dec-19 14:13 UTC

[PATCH RFC 2/5] s390: add pci_iomap_range

On Mon, 15 Dec 2014, Michael S. Tsirkin wrote:> From: Michael S Tsirkin <mst at redhat.com>
> 
> Virtio drivers should map the part of the range they need, not
> necessarily all of it.
> To this end, support mapping ranges within BAR on s390.
> Since multiple ranges can now be mapped within a BAR, we keep track of
> the number of mappings created, and only clear out the mapping for a BAR
> when this number reaches 0.
> 
I can't say much about the users of this interface but in principle I'm
OK with such a change.
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  arch/s390/include/asm/pci_io.h |  1 +
>  arch/s390/pci/pci.c            | 34 +++++++++++++++++++++++++++-------
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci_io.h
b/arch/s390/include/asm/pci_io.h
> index d194d54..25228b3 100644
> --- a/arch/s390/include/asm/pci_io.h
> +++ b/arch/s390/include/asm/pci_io.h
> @@ -16,6 +16,7 @@
>  struct zpci_iomap_entry {
>  	u32 fh;
>  	u8 bar;
> +	u16 count;
>  };
> 
>  extern struct zpci_iomap_entry *zpci_iomap_start;
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 2fa7b14..51cb653 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const void
*from, size_t count)
>  }
> 
>  /* Create a virtual mapping cookie for a PCI BAR */
> -void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
> +void __iomem *pci_iomap_range(struct pci_dev *pdev,
> +			      int bar,
> +			      unsigned long offset,
> +			      unsigned long max)
>  {
>  	struct zpci_dev *zdev =	get_zdev(pdev);
>  	u64 addr;
> @@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev, int
bar, unsigned long max)
> 
>  	idx = zdev->bars[bar].map_idx;
>  	spin_lock(&zpci_iomap_lock);
> -	zpci_iomap_start[idx].fh = zdev->fh;
> -	zpci_iomap_start[idx].bar = bar;
> +	if (zpci_iomap_start[idx].count++) {
> +		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
> +		       zpci_iomap_start[idx].bar != bar);
> +	} else {
> +		zpci_iomap_start[idx].fh = zdev->fh;
> +		zpci_iomap_start[idx].bar = bar;
> +	}
> +	/* Detect overrun */
> +	BUG_ON(!zpci_iomap_start[idx].count);
>  	spin_unlock(&zpci_iomap_lock);
> 
>  	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
> -	return (void __iomem *) addr;
> +	return (void __iomem *) addr + offset;
>  }
> -EXPORT_SYMBOL_GPL(pci_iomap);
> +EXPORT_SYMBOL_GPL(pci_iomap_range);
> +
> +void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long
maxlen)
> +{
> +	return pci_iomap_range(dev, bar, 0, maxlen);
> +}
> +EXPORT_SYMBOL(pci_iomap);
This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay that
way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like we
should use that too.

Regards,
Sebastian
> 
>  void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
>  {
> @@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void __iomem
*addr)
> 
>  	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >> 48;
>  	spin_lock(&zpci_iomap_lock);
> -	zpci_iomap_start[idx].fh = 0;
> -	zpci_iomap_start[idx].bar = 0;
> +	/* Detect underrun */
> +	BUG_ON(!zpci_iomap_start[idx].count);
> +	if (!--zpci_iomap_start[idx].count) {
> +		zpci_iomap_start[idx].fh = 0;
> +		zpci_iomap_start[idx].bar = 0;
> +	}
>  	spin_unlock(&zpci_iomap_lock);
>  }
>  EXPORT_SYMBOL_GPL(pci_iounmap);
> -- 
> MST
> 
>

Michael S. Tsirkin

2014-Dec-19 14:28 UTC

head link

[PATCH RFC 2/5] s390: add pci_iomap_range

On Fri, Dec 19, 2014 at 03:13:37PM +0100, Sebastian Ott
wrote:> On Mon, 15 Dec 2014, Michael S. Tsirkin wrote:
> > From: Michael S Tsirkin <mst at redhat.com>
> > 
> > Virtio drivers should map the part of the range they need, not
> > necessarily all of it.
> > To this end, support mapping ranges within BAR on s390.
> > Since multiple ranges can now be mapped within a BAR, we keep track of
> > the number of mappings created, and only clear out the mapping for a
BAR
> > when this number reaches 0.
> > 
> 
> I can't say much about the users of this interface but in principle
I'm
> OK with such a change.
I don't have an s390 system with pci for testing - could you help me out
by testing this and confirming it doesn't break things?
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > ---
> >  arch/s390/include/asm/pci_io.h |  1 +
> >  arch/s390/pci/pci.c            | 34
+++++++++++++++++++++++++++-------
> >  2 files changed, 28 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/pci_io.h
b/arch/s390/include/asm/pci_io.h
> > index d194d54..25228b3 100644
> > --- a/arch/s390/include/asm/pci_io.h
> > +++ b/arch/s390/include/asm/pci_io.h
> > @@ -16,6 +16,7 @@
> >  struct zpci_iomap_entry {
> >  	u32 fh;
> >  	u8 bar;
> > +	u16 count;
> >  };
> > 
> >  extern struct zpci_iomap_entry *zpci_iomap_start;
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index 2fa7b14..51cb653 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const
void *from, size_t count)
> >  }
> > 
> >  /* Create a virtual mapping cookie for a PCI BAR */
> > -void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long
max)
> > +void __iomem *pci_iomap_range(struct pci_dev *pdev,
> > +			      int bar,
> > +			      unsigned long offset,
> > +			      unsigned long max)
> >  {
> >  	struct zpci_dev *zdev =	get_zdev(pdev);
> >  	u64 addr;
> > @@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev,
int bar, unsigned long max)
> > 
> >  	idx = zdev->bars[bar].map_idx;
> >  	spin_lock(&zpci_iomap_lock);
> > -	zpci_iomap_start[idx].fh = zdev->fh;
> > -	zpci_iomap_start[idx].bar = bar;
> > +	if (zpci_iomap_start[idx].count++) {
> > +		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
> > +		       zpci_iomap_start[idx].bar != bar);
> > +	} else {
> > +		zpci_iomap_start[idx].fh = zdev->fh;
> > +		zpci_iomap_start[idx].bar = bar;
> > +	}
> > +	/* Detect overrun */
> > +	BUG_ON(!zpci_iomap_start[idx].count);
> >  	spin_unlock(&zpci_iomap_lock);
> > 
> >  	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
> > -	return (void __iomem *) addr;
> > +	return (void __iomem *) addr + offset;
> >  }
> > -EXPORT_SYMBOL_GPL(pci_iomap);
> > +EXPORT_SYMBOL_GPL(pci_iomap_range);
> > +
> > +void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long
maxlen)
> > +{
> > +	return pci_iomap_range(dev, bar, 0, maxlen);
> > +}
> > +EXPORT_SYMBOL(pci_iomap);
> 
> This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay that
> way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like we
> should use that too.
> 
> Regards,
> Sebastian
OK, so you want two patches: one with new functionality,
one switching to EXPORT_SYMBOL?

> > 
> >  void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
> >  {
> > @@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void
__iomem *addr)
> > 
> >  	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >>
48;
> >  	spin_lock(&zpci_iomap_lock);
> > -	zpci_iomap_start[idx].fh = 0;
> > -	zpci_iomap_start[idx].bar = 0;
> > +	/* Detect underrun */
> > +	BUG_ON(!zpci_iomap_start[idx].count);
> > +	if (!--zpci_iomap_start[idx].count) {
> > +		zpci_iomap_start[idx].fh = 0;
> > +		zpci_iomap_start[idx].bar = 0;
> > +	}
> >  	spin_unlock(&zpci_iomap_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_iounmap);
> > -- 
> > MST
> > 
> >

Sebastian Ott

2014-Dec-19 15:13 UTC

head link

[PATCH RFC 2/5] s390: add pci_iomap_range

On Fri, 19 Dec 2014, Michael S. Tsirkin wrote:> On Fri, Dec 19, 2014 at 03:13:37PM +0100, Sebastian Ott wrote:
> > On Mon, 15 Dec 2014, Michael S. Tsirkin wrote:
> > > From: Michael S Tsirkin <mst at redhat.com>
> > > 
> > > Virtio drivers should map the part of the range they need, not
> > > necessarily all of it.
> > > To this end, support mapping ranges within BAR on s390.
> > > Since multiple ranges can now be mapped within a BAR, we keep
track of
> > > the number of mappings created, and only clear out the mapping
for a BAR
> > > when this number reaches 0.
> > > 
> > 
> > I can't say much about the users of this interface but in
principle I'm
> > OK with such a change.
> 
> I don't have an s390 system with pci for testing - could you help me
out
> by testing this and confirming it doesn't break things?
I did some minor tests and nothing broke...
> 
> > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > > ---
> > >  arch/s390/include/asm/pci_io.h |  1 +
> > >  arch/s390/pci/pci.c            | 34
+++++++++++++++++++++++++++-------
> > >  2 files changed, 28 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/s390/include/asm/pci_io.h
b/arch/s390/include/asm/pci_io.h
> > > index d194d54..25228b3 100644
> > > --- a/arch/s390/include/asm/pci_io.h
> > > +++ b/arch/s390/include/asm/pci_io.h
> > > @@ -16,6 +16,7 @@
> > >  struct zpci_iomap_entry {
> > >  	u32 fh;
> > >  	u8 bar;
> > > +	u16 count;
> > >  };
> > > 
> > >  extern struct zpci_iomap_entry *zpci_iomap_start;
> > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > index 2fa7b14..51cb653 100644
> > > --- a/arch/s390/pci/pci.c
> > > +++ b/arch/s390/pci/pci.c
> > > @@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to,
const void *from, size_t count)
> > >  }
> > > 
> > >  /* Create a virtual mapping cookie for a PCI BAR */
> > > -void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned
long max)
> > > +void __iomem *pci_iomap_range(struct pci_dev *pdev,
> > > +			      int bar,
> > > +			      unsigned long offset,
> > > +			      unsigned long max)
> > >  {
> > >  	struct zpci_dev *zdev =	get_zdev(pdev);
> > >  	u64 addr;
> > > @@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev
*pdev, int bar, unsigned long max)
> > > 
> > >  	idx = zdev->bars[bar].map_idx;
> > >  	spin_lock(&zpci_iomap_lock);
> > > -	zpci_iomap_start[idx].fh = zdev->fh;
> > > -	zpci_iomap_start[idx].bar = bar;
> > > +	if (zpci_iomap_start[idx].count++) {
> > > +		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
> > > +		       zpci_iomap_start[idx].bar != bar);
> > > +	} else {
> > > +		zpci_iomap_start[idx].fh = zdev->fh;
> > > +		zpci_iomap_start[idx].bar = bar;
> > > +	}
> > > +	/* Detect overrun */
> > > +	BUG_ON(!zpci_iomap_start[idx].count);
> > >  	spin_unlock(&zpci_iomap_lock);
> > > 
> > >  	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
> > > -	return (void __iomem *) addr;
> > > +	return (void __iomem *) addr + offset;
> > >  }
> > > -EXPORT_SYMBOL_GPL(pci_iomap);
> > > +EXPORT_SYMBOL_GPL(pci_iomap_range);
> > > +
> > > +void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned
long maxlen)
> > > +{
> > > +	return pci_iomap_range(dev, bar, 0, maxlen);
> > > +}
> > > +EXPORT_SYMBOL(pci_iomap);
> > 
> > This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay
that
> > way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like
we
> > should use that too.
> > 
> > Regards,
> > Sebastian
> 
> OK, so you want two patches: one with new functionality,
> one switching to EXPORT_SYMBOL?
Either that or at least add a note in the patch description.

Regards,
Sebastian
> 
> 
> > > 
> > >  void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
> > >  {
> > > @@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void
__iomem *addr)
> > > 
> > >  	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE)
>> 48;
> > >  	spin_lock(&zpci_iomap_lock);
> > > -	zpci_iomap_start[idx].fh = 0;
> > > -	zpci_iomap_start[idx].bar = 0;
> > > +	/* Detect underrun */
> > > +	BUG_ON(!zpci_iomap_start[idx].count);
> > > +	if (!--zpci_iomap_start[idx].count) {
> > > +		zpci_iomap_start[idx].fh = 0;
> > > +		zpci_iomap_start[idx].bar = 0;
> > > +	}
> > >  	spin_unlock(&zpci_iomap_lock);
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_iounmap);
> > > -- 
> > > MST
> > > 
> > > 
> 
>

Possibly Parallel Threads

Search for more reasonably related threads

Virtualization - Dec 2014 - [PATCH RFC 2/5] s390: add pci_iomap_range

[PATCH RFC 2/5] s390: add pci_iomap_range

[PATCH RFC 2/5] s390: add pci_iomap_range

[PATCH RFC 2/5] s390: add pci_iomap_range

Possibly Parallel Threads