Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
@ 2019-09-11  5:17 Seunghun Han
  2019-09-13 14:00 ` Jarkko Sakkinen
  0 siblings, 1 reply; 10+ messages in thread
From: Seunghun Han @ 2019-09-11  5:17 UTC (permalink / raw)
  To: ivan.lazeev
  Cc: arnd, gregkh, jarkko.sakkinen, jgg, linux-integrity,
	linux-kernel, peterhuewe

> > And why is this allocating memory inside the acpi table walker? It
> > seems to me like the memory should be allocated once the mapping is
> > made.
> >
>
> Yes, this looks bad. Letting the walker build the list and then using
> it is, probably, a better idea.
>
> > Maybe all the mappings should be created from the ACPI ranges right
> > away?
> >
>
> I don't know if it's a good idea to just map them all instead of doing
> so only for relevant ones. Maybe it is safe, here I need an advice
> from a more knowledgeable person.
>

Vanya,
I also made a patch series to solve AMD's fTPM. My patch link is here,
https://lkml.org/lkml/2019/9/9/132 .

The maintainer, Jarkko, wanted me to remark on your patch, so I would
like to cooperate with you.

Your patch is good for me. If you are fine, I would like to take your
patch and merge it with my patch series. I also would like to change
some points Jason mentioned before.

Of course, I will leave your commit message and sign-off-by note.
According to the guideline below, I will just add co-developed-by and
sign-off-by notes behind you.
https://www.kernel.org/doc/html/v5.2/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

If you have any idea about our co-work, please let me know.
I hope we can solve AMD's fTPM problem soon.

Seunghun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-09-11  5:17 [PATCH v2] Fix fTPM on AMD Zen+ CPUs Seunghun Han
@ 2019-09-13 14:00 ` Jarkko Sakkinen
  2019-09-13 14:02   ` Jarkko Sakkinen
  0 siblings, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-09-13 14:00 UTC (permalink / raw)
  To: Seunghun Han
  Cc: ivan.lazeev, arnd, gregkh, jgg, linux-integrity, linux-kernel,
	peterhuewe

On Wed, Sep 11, 2019 at 02:17:48PM +0900, Seunghun Han wrote:
> Vanya,
> I also made a patch series to solve AMD's fTPM. My patch link is here,
> https://lkml.org/lkml/2019/9/9/132 .
> 
> The maintainer, Jarkko, wanted me to remark on your patch, so I would
> like to cooperate with you.
> 
> Your patch is good for me. If you are fine, I would like to take your
> patch and merge it with my patch series. I also would like to change
> some points Jason mentioned before.

I rather handle the review processes separately because I can merge
Vanyas's patch first. Bundling them into patch set would only slow
down things.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-09-13 14:00 ` Jarkko Sakkinen
@ 2019-09-13 14:02   ` Jarkko Sakkinen
  2019-09-16  5:29     ` Seunghun Han
  0 siblings, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-09-13 14:02 UTC (permalink / raw)
  To: Seunghun Han
  Cc: ivan.lazeev, arnd, gregkh, jgg, linux-integrity, linux-kernel,
	peterhuewe

On Fri, Sep 13, 2019 at 03:00:06PM +0100, Jarkko Sakkinen wrote:
> On Wed, Sep 11, 2019 at 02:17:48PM +0900, Seunghun Han wrote:
> > Vanya,
> > I also made a patch series to solve AMD's fTPM. My patch link is here,
> > https://lkml.org/lkml/2019/9/9/132 .
> > 
> > The maintainer, Jarkko, wanted me to remark on your patch, so I would
> > like to cooperate with you.
> > 
> > Your patch is good for me. If you are fine, I would like to take your
> > patch and merge it with my patch series. I also would like to change
> > some points Jason mentioned before.
> 
> I rather handle the review processes separately because I can merge
> Vanyas's patch first. Bundling them into patch set would only slow
> down things.

I did not ask to do anything. I just review code changes.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-09-13 14:02   ` Jarkko Sakkinen
@ 2019-09-16  5:29     ` Seunghun Han
  2019-09-16  9:07       ` Jarkko Sakkinen
  0 siblings, 1 reply; 10+ messages in thread
From: Seunghun Han @ 2019-09-16  5:29 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Vanya Lazeev, arnd, Greg Kroah-Hartman, Jason Gunthorpe,
	open list:TPM DEVICE DRIVER, Linux Kernel Mailing List,
	Peter Huewe

>
> On Fri, Sep 13, 2019 at 03:00:06PM +0100, Jarkko Sakkinen wrote:
> > On Wed, Sep 11, 2019 at 02:17:48PM +0900, Seunghun Han wrote:
> > > Vanya,
> > > I also made a patch series to solve AMD's fTPM. My patch link is here,
> > > https://lkml.org/lkml/2019/9/9/132 .
> > >
> > > The maintainer, Jarkko, wanted me to remark on your patch, so I would
> > > like to cooperate with you.
> > >
> > > Your patch is good for me. If you are fine, I would like to take your
> > > patch and merge it with my patch series. I also would like to change
> > > some points Jason mentioned before.
> >
> > I rather handle the review processes separately because I can merge
> > Vanyas's patch first. Bundling them into patch set would only slow
> > down things.
>
> I did not ask to do anything. I just review code changes.

I got it. I should concentrate on my ACPI NVS problem.
Thank you.

Seunghun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-09-16  5:29     ` Seunghun Han
@ 2019-09-16  9:07       ` Jarkko Sakkinen
  2019-09-16  9:43         ` Seunghun Han
  0 siblings, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-09-16  9:07 UTC (permalink / raw)
  To: Seunghun Han
  Cc: Vanya Lazeev, arnd, Greg Kroah-Hartman, Jason Gunthorpe,
	open list:TPM DEVICE DRIVER, Linux Kernel Mailing List,
	Peter Huewe

On Mon, Sep 16, 2019 at 02:29:01PM +0900, Seunghun Han wrote:
> >
> > On Fri, Sep 13, 2019 at 03:00:06PM +0100, Jarkko Sakkinen wrote:
> > > On Wed, Sep 11, 2019 at 02:17:48PM +0900, Seunghun Han wrote:
> > > > Vanya,
> > > > I also made a patch series to solve AMD's fTPM. My patch link is here,
> > > > https://lkml.org/lkml/2019/9/9/132 .
> > > >
> > > > The maintainer, Jarkko, wanted me to remark on your patch, so I would
> > > > like to cooperate with you.
> > > >
> > > > Your patch is good for me. If you are fine, I would like to take your
> > > > patch and merge it with my patch series. I also would like to change
> > > > some points Jason mentioned before.
> > >
> > > I rather handle the review processes separately because I can merge
> > > Vanyas's patch first. Bundling them into patch set would only slow
> > > down things.
> >
> > I did not ask to do anything. I just review code changes.
> 
> I got it. I should concentrate on my ACPI NVS problem.
> Thank you.

Eessentially what you want to do is to detach and backup the original
NVS resources and put them back to the list with insert_resource() when
tpm_crb is removed. At least I think this is what should be done but you
should CC your patch also to the ACPI list for feedback.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-09-16  9:07       ` Jarkko Sakkinen
@ 2019-09-16  9:43         ` Seunghun Han
  0 siblings, 0 replies; 10+ messages in thread
From: Seunghun Han @ 2019-09-16  9:43 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Vanya Lazeev, arnd, Greg Kroah-Hartman, Jason Gunthorpe,
	open list:TPM DEVICE DRIVER, Linux Kernel Mailing List,
	Peter Huewe

> Eessentially what you want to do is to detach and backup the original
> NVS resources and put them back to the list with insert_resource() when
> tpm_crb is removed. At least I think this is what should be done but you
> should CC your patch also to the ACPI list for feedback.
>
> /Jarkko

Yes, you are right. But, what I really want to do is requesting
command/response buffer regions from NVS driver and releasing them. To
detach and backup the original NVS resources with insert_resource() or
remove_resource() are not needed maybe.

I have some idea about it, so I have sent an email to you. Would you
check the email and give comments? The link is here,
https://lkml.org/lkml/2019/9/16/112 .

Seunghun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-08-12 22:42   ` Vanya Lazeev
@ 2019-08-15 20:47     ` Jarkko Sakkinen
  0 siblings, 0 replies; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-08-15 20:47 UTC (permalink / raw)
  To: Vanya Lazeev
  Cc: Jason Gunthorpe, Peter Huewe, Arnd Bergmann, Greg Kroah-Hartman,
	linux-integrity, linux-kernel

On Tue, Aug 13, 2019 at 01:42:42AM +0300, Vanya Lazeev wrote:
> fTPM on Zen+ not only needs multiple mappings, it can also return
> inconsistent with ACPI values for range sizes (as for me and
> mikajhe from the bug thread), so results of crb_containing_resource
> are also used to fix the inconsistencies with crb_fixup_cmd_size.

How do you think that just by your code change, without any explanation
in the commit message, we could backtrack all this information?

/Jarko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-08-12 13:10 ` Jason Gunthorpe
@ 2019-08-12 22:42   ` Vanya Lazeev
  2019-08-15 20:47     ` Jarkko Sakkinen
  0 siblings, 1 reply; 10+ messages in thread
From: Vanya Lazeev @ 2019-08-12 22:42 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Jarkko Sakkinen, Peter Huewe, Arnd Bergmann, Greg Kroah-Hartman,
	linux-integrity, linux-kernel

On Mon, Aug 12, 2019 at 10:10:03AM -0300, Jason Gunthorpe wrote:
> On Sun, Aug 11, 2019 at 09:52:18PM +0300, ivan.lazeev@gmail.com wrote:
> > From: Vanya Lazeev <ivan.lazeev@gmail.com>
> > 
> > The patch is an attempt to make fTPM on AMD Zen CPUs work.
> > Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657
> > 
> > The problem seems to be that tpm_crb driver doesn't expect tpm command
> > and response memory regions to belong to different ACPI resources.
> > 
> > Tested on Asrock ITX motherboard with Ryzen 2600X CPU.
> > However, I don't have any other hardware to test the changes on and no
> > expertise to be sure that other TPMs won't break as a result.
> > Hopefully, the patch will be useful.
> > 
> > Changes from v1:
> > - use list_for_each_safe
> > 
> > Signed-off-by: Vanya Lazeev <ivan.lazeev@gmail.com>
> >  drivers/char/tpm/tpm_crb.c | 146 ++++++++++++++++++++++++++++---------
> >  1 file changed, 110 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index e59f1f91d..b0e797464 100644
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -91,7 +91,6 @@ enum crb_status {
> >  struct crb_priv {
> >  	u32 sm;
> >  	const char *hid;
> > -	void __iomem *iobase;
> >  	struct crb_regs_head __iomem *regs_h;
> >  	struct crb_regs_tail __iomem *regs_t;
> >  	u8 __iomem *cmd;
> > @@ -108,6 +107,13 @@ struct tpm2_crb_smc {
> >  	u32 smc_func_id;
> >  };
> >  
> > +struct crb_resource {
> > +	struct resource io_res;
> > +	void __iomem *iobase;
> > +
> > +	struct list_head link;
> > +};
> > +
> >  static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
> >  				unsigned long timeout)
> >  {
> > @@ -432,23 +438,69 @@ static const struct tpm_class_ops tpm_crb = {
> >  	.req_complete_val = CRB_DRV_STS_COMPLETE,
> >  };
> >  
> > +static void crb_free_resource_list(struct list_head *resources)
> > +{
> > +	struct list_head *position, *tmp;
> > +
> > +	list_for_each_safe(position, tmp, resources)
> > +		kfree(list_entry(position, struct crb_resource, link));
> > +}
> > +
> > +/**
> > + * Checks if resource @io_res contains range with the specified @start and @size
> > + * completely or, when @strict is false, at least it's beginning.
> > + * Non-strict match is needed to work around broken BIOSes that return
> > + * inconsistent values from ACPI regions vs registers.
> > + */
> > +static inline bool crb_resource_contains(const struct resource *io_res,
> > +					 u64 start, u32 size, bool strict)
> > +{
> > +	return start >= io_res->start &&
> > +		(start + size - 1 <= io_res->end ||
> > +		 (!strict && start <= io_res->end));
> > +}
> > +
> > +static struct crb_resource *crb_containing_resource(
> > +		const struct list_head *resources,
> > +		u64 start, u32 size, bool strict)
> > +{
> > +	struct list_head *pos;
> > +
> > +	list_for_each(pos, resources) {
> > +		struct crb_resource *cres;
> > +
> > +		cres = list_entry(pos, struct crb_resource, link);
> > +		if (crb_resource_contains(&cres->io_res, start, size, strict))
> > +			return cres;
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> This flow seems very strange, why isn't this part of crb_map_res?
> 

fTPM on Zen+ not only needs multiple mappings, it can also return
inconsistent with ACPI values for range sizes (as for me and
mikajhe from the bug thread), so results of crb_containing_resource 
are also used to fix the inconsistencies with crb_fixup_cmd_size.

> >  static int crb_check_resource(struct acpi_resource *ares, void *data)
> >  {
> > -	struct resource *io_res = data;
> > +	struct list_head *list = data;
> > +	struct crb_resource *cres;
> >  	struct resource_win win;
> >  	struct resource *res = &(win.res);
> >  
> >  	if (acpi_dev_resource_memory(ares, res) ||
> >  	    acpi_dev_resource_address_space(ares, &win)) {
> > -		*io_res = *res;
> > -		io_res->name = NULL;
> > +		cres = kzalloc(sizeof(*cres), GFP_KERNEL);
> > +		if (!cres)
> > +			return -ENOMEM;
> > +
> > +		cres->io_res = *res;
> > +		cres->io_res.name = NULL;
> > +
> > +		list_add_tail(&cres->link, list);
> 
> And why is this allocating memory inside the acpi table walker? It
> seems to me like the memory should be allocated once the mapping is
> made.
> 

Yes, this looks bad. Letting the walker build the list and then using
it is, probably, a better idea.

> Maybe all the mappings should be created from the ACPI ranges right
> away?
> 

I don't know if it's a good idea to just map them all instead of doing 
so only for relevant ones. Maybe it is safe, here I need an advice
from a more knowledgeable person.

> > @@ -460,10 +512,16 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> >  	if (start != new_res.start)
> >  		return (void __iomem *) ERR_PTR(-EINVAL);
> >  
> > -	if (!resource_contains(io_res, &new_res))
> > +	if (!cres)
> >  		return devm_ioremap_resource(dev, &new_res);
> >  
> > -	return priv->iobase + (new_res.start - io_res->start);
> > +	if (!cres->iobase) {
> > +		cres->iobase = devm_ioremap_resource(dev, &cres->io_res);
> > +		if (IS_ERR(cres->iobase))
> > +			return cres->iobase;
> > +	}
> 
> It sounds likethe real bug is that the crb_map_res only considers a
> single active mapping, while these AMD chips need more than one?
> 

Yes, this seems to be the issue.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] Fix fTPM on AMD Zen+ CPUs
  2019-08-11 18:52 ivan.lazeev
@ 2019-08-12 13:10 ` Jason Gunthorpe
  2019-08-12 22:42   ` Vanya Lazeev
  0 siblings, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2019-08-12 13:10 UTC (permalink / raw)
  To: ivan.lazeev
  Cc: Jarkko Sakkinen, Peter Huewe, Arnd Bergmann, Greg Kroah-Hartman,
	linux-integrity, linux-kernel

On Sun, Aug 11, 2019 at 09:52:18PM +0300, ivan.lazeev@gmail.com wrote:
> From: Vanya Lazeev <ivan.lazeev@gmail.com>
> 
> The patch is an attempt to make fTPM on AMD Zen CPUs work.
> Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657
> 
> The problem seems to be that tpm_crb driver doesn't expect tpm command
> and response memory regions to belong to different ACPI resources.
> 
> Tested on Asrock ITX motherboard with Ryzen 2600X CPU.
> However, I don't have any other hardware to test the changes on and no
> expertise to be sure that other TPMs won't break as a result.
> Hopefully, the patch will be useful.
> 
> Changes from v1:
> - use list_for_each_safe
> 
> Signed-off-by: Vanya Lazeev <ivan.lazeev@gmail.com>
>  drivers/char/tpm/tpm_crb.c | 146 ++++++++++++++++++++++++++++---------
>  1 file changed, 110 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d..b0e797464 100644
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -91,7 +91,6 @@ enum crb_status {
>  struct crb_priv {
>  	u32 sm;
>  	const char *hid;
> -	void __iomem *iobase;
>  	struct crb_regs_head __iomem *regs_h;
>  	struct crb_regs_tail __iomem *regs_t;
>  	u8 __iomem *cmd;
> @@ -108,6 +107,13 @@ struct tpm2_crb_smc {
>  	u32 smc_func_id;
>  };
>  
> +struct crb_resource {
> +	struct resource io_res;
> +	void __iomem *iobase;
> +
> +	struct list_head link;
> +};
> +
>  static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>  				unsigned long timeout)
>  {
> @@ -432,23 +438,69 @@ static const struct tpm_class_ops tpm_crb = {
>  	.req_complete_val = CRB_DRV_STS_COMPLETE,
>  };
>  
> +static void crb_free_resource_list(struct list_head *resources)
> +{
> +	struct list_head *position, *tmp;
> +
> +	list_for_each_safe(position, tmp, resources)
> +		kfree(list_entry(position, struct crb_resource, link));
> +}
> +
> +/**
> + * Checks if resource @io_res contains range with the specified @start and @size
> + * completely or, when @strict is false, at least it's beginning.
> + * Non-strict match is needed to work around broken BIOSes that return
> + * inconsistent values from ACPI regions vs registers.
> + */
> +static inline bool crb_resource_contains(const struct resource *io_res,
> +					 u64 start, u32 size, bool strict)
> +{
> +	return start >= io_res->start &&
> +		(start + size - 1 <= io_res->end ||
> +		 (!strict && start <= io_res->end));
> +}
> +
> +static struct crb_resource *crb_containing_resource(
> +		const struct list_head *resources,
> +		u64 start, u32 size, bool strict)
> +{
> +	struct list_head *pos;
> +
> +	list_for_each(pos, resources) {
> +		struct crb_resource *cres;
> +
> +		cres = list_entry(pos, struct crb_resource, link);
> +		if (crb_resource_contains(&cres->io_res, start, size, strict))
> +			return cres;
> +	}
> +
> +	return NULL;
> +}

This flow seems very strange, why isn't this part of crb_map_res?

>  static int crb_check_resource(struct acpi_resource *ares, void *data)
>  {
> -	struct resource *io_res = data;
> +	struct list_head *list = data;
> +	struct crb_resource *cres;
>  	struct resource_win win;
>  	struct resource *res = &(win.res);
>  
>  	if (acpi_dev_resource_memory(ares, res) ||
>  	    acpi_dev_resource_address_space(ares, &win)) {
> -		*io_res = *res;
> -		io_res->name = NULL;
> +		cres = kzalloc(sizeof(*cres), GFP_KERNEL);
> +		if (!cres)
> +			return -ENOMEM;
> +
> +		cres->io_res = *res;
> +		cres->io_res.name = NULL;
> +
> +		list_add_tail(&cres->link, list);

And why is this allocating memory inside the acpi table walker? It
seems to me like the memory should be allocated once the mapping is
made.

Maybe all the mappings should be created from the ACPI ranges right
away?

> @@ -460,10 +512,16 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
>  	if (start != new_res.start)
>  		return (void __iomem *) ERR_PTR(-EINVAL);
>  
> -	if (!resource_contains(io_res, &new_res))
> +	if (!cres)
>  		return devm_ioremap_resource(dev, &new_res);
>  
> -	return priv->iobase + (new_res.start - io_res->start);
> +	if (!cres->iobase) {
> +		cres->iobase = devm_ioremap_resource(dev, &cres->io_res);
> +		if (IS_ERR(cres->iobase))
> +			return cres->iobase;
> +	}

It sounds likethe real bug is that the crb_map_res only considers a
single active mapping, while these AMD chips need more than one?

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] Fix fTPM on AMD Zen+ CPUs
@ 2019-08-11 18:52 ivan.lazeev
  2019-08-12 13:10 ` Jason Gunthorpe
  0 siblings, 1 reply; 10+ messages in thread
From: ivan.lazeev @ 2019-08-11 18:52 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	linux-integrity, linux-kernel, Vanya Lazeev

From: Vanya Lazeev <ivan.lazeev@gmail.com>

The patch is an attempt to make fTPM on AMD Zen CPUs work.
Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657

The problem seems to be that tpm_crb driver doesn't expect tpm command
and response memory regions to belong to different ACPI resources.

Tested on Asrock ITX motherboard with Ryzen 2600X CPU.
However, I don't have any other hardware to test the changes on and no
expertise to be sure that other TPMs won't break as a result.
Hopefully, the patch will be useful.

Changes from v1:
- use list_for_each_safe

Signed-off-by: Vanya Lazeev <ivan.lazeev@gmail.com>
---
 drivers/char/tpm/tpm_crb.c | 146 ++++++++++++++++++++++++++++---------
 1 file changed, 110 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index e59f1f91d..b0e797464 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -91,7 +91,6 @@ enum crb_status {
 struct crb_priv {
 	u32 sm;
 	const char *hid;
-	void __iomem *iobase;
 	struct crb_regs_head __iomem *regs_h;
 	struct crb_regs_tail __iomem *regs_t;
 	u8 __iomem *cmd;
@@ -108,6 +107,13 @@ struct tpm2_crb_smc {
 	u32 smc_func_id;
 };
 
+struct crb_resource {
+	struct resource io_res;
+	void __iomem *iobase;
+
+	struct list_head link;
+};
+
 static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
 				unsigned long timeout)
 {
@@ -432,23 +438,69 @@ static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
 
+static void crb_free_resource_list(struct list_head *resources)
+{
+	struct list_head *position, *tmp;
+
+	list_for_each_safe(position, tmp, resources)
+		kfree(list_entry(position, struct crb_resource, link));
+}
+
+/**
+ * Checks if resource @io_res contains range with the specified @start and @size
+ * completely or, when @strict is false, at least it's beginning.
+ * Non-strict match is needed to work around broken BIOSes that return
+ * inconsistent values from ACPI regions vs registers.
+ */
+static inline bool crb_resource_contains(const struct resource *io_res,
+					 u64 start, u32 size, bool strict)
+{
+	return start >= io_res->start &&
+		(start + size - 1 <= io_res->end ||
+		 (!strict && start <= io_res->end));
+}
+
+static struct crb_resource *crb_containing_resource(
+		const struct list_head *resources,
+		u64 start, u32 size, bool strict)
+{
+	struct list_head *pos;
+
+	list_for_each(pos, resources) {
+		struct crb_resource *cres;
+
+		cres = list_entry(pos, struct crb_resource, link);
+		if (crb_resource_contains(&cres->io_res, start, size, strict))
+			return cres;
+	}
+
+	return NULL;
+}
+
 static int crb_check_resource(struct acpi_resource *ares, void *data)
 {
-	struct resource *io_res = data;
+	struct list_head *list = data;
+	struct crb_resource *cres;
 	struct resource_win win;
 	struct resource *res = &(win.res);
 
 	if (acpi_dev_resource_memory(ares, res) ||
 	    acpi_dev_resource_address_space(ares, &win)) {
-		*io_res = *res;
-		io_res->name = NULL;
+		cres = kzalloc(sizeof(*cres), GFP_KERNEL);
+		if (!cres)
+			return -ENOMEM;
+
+		cres->io_res = *res;
+		cres->io_res.name = NULL;
+
+		list_add_tail(&cres->link, list);
 	}
 
 	return 1;
 }
 
-static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
-				 struct resource *io_res, u64 start, u32 size)
+static void __iomem *crb_map_res(struct device *dev, struct crb_resource *cres,
+				 u64 start, u32 size)
 {
 	struct resource new_res = {
 		.start	= start,
@@ -460,10 +512,16 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
 	if (start != new_res.start)
 		return (void __iomem *) ERR_PTR(-EINVAL);
 
-	if (!resource_contains(io_res, &new_res))
+	if (!cres)
 		return devm_ioremap_resource(dev, &new_res);
 
-	return priv->iobase + (new_res.start - io_res->start);
+	if (!cres->iobase) {
+		cres->iobase = devm_ioremap_resource(dev, &cres->io_res);
+		if (IS_ERR(cres->iobase))
+			return cres->iobase;
+	}
+
+	return cres->iobase + (new_res.start - cres->io_res.start);
 }
 
 /*
@@ -490,9 +548,9 @@ static u64 crb_fixup_cmd_size(struct device *dev, struct resource *io_res,
 static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		      struct acpi_table_tpm2 *buf)
 {
-	struct list_head resources;
-	struct resource io_res;
+	struct list_head acpi_resources, crb_resources;
 	struct device *dev = &device->dev;
+	struct crb_resource *cres;
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
@@ -501,21 +559,30 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	u32 rsp_size;
 	int ret;
 
-	INIT_LIST_HEAD(&resources);
-	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
-				     &io_res);
+	INIT_LIST_HEAD(&acpi_resources);
+	INIT_LIST_HEAD(&crb_resources);
+	ret = acpi_dev_get_resources(device, &acpi_resources,
+				     crb_check_resource, &crb_resources);
 	if (ret < 0)
-		return ret;
-	acpi_dev_free_resource_list(&resources);
+		goto out_free_crb_resources;
+
+	acpi_dev_free_resource_list(&acpi_resources);
 
-	if (resource_type(&io_res) != IORESOURCE_MEM) {
+	if (list_empty(&crb_resources)) {
 		dev_err(dev, FW_BUG "TPM2 ACPI table does not define a memory resource\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free_crb_resources;
 	}
 
-	priv->iobase = devm_ioremap_resource(dev, &io_res);
-	if (IS_ERR(priv->iobase))
-		return PTR_ERR(priv->iobase);
+	cres = crb_containing_resource(&crb_resources, buf->control_address,
+				       sizeof(struct crb_regs_tail), true);
+
+	priv->regs_t = crb_map_res(dev, cres, buf->control_address,
+				   sizeof(struct crb_regs_tail));
+	if (IS_ERR(priv->regs_t)) {
+		ret = PTR_ERR(priv->regs_t);
+		goto out_free_crb_resources;
+	}
 
 	/* The ACPI IO region starts at the head area and continues to include
 	 * the control area, as one nice sane region except for some older
@@ -523,23 +590,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 */
 	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
 	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
-		if (buf->control_address == io_res.start +
+		if (cres &&
+		    buf->control_address == cres->io_res.start +
 		    sizeof(*priv->regs_h))
-			priv->regs_h = priv->iobase;
+			priv->regs_h = cres->iobase;
 		else
 			dev_warn(dev, FW_BUG "Bad ACPI memory layout");
 	}
 
 	ret = __crb_request_locality(dev, priv, 0);
 	if (ret)
-		return ret;
-
-	priv->regs_t = crb_map_res(dev, priv, &io_res, buf->control_address,
-				   sizeof(struct crb_regs_tail));
-	if (IS_ERR(priv->regs_t)) {
-		ret = PTR_ERR(priv->regs_t);
-		goto out_relinquish_locality;
-	}
+		goto out_free_crb_resources;
 
 	/*
 	 * PTT HW bug w/a: wake up the device to access
@@ -552,13 +613,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
 	pa_low  = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
 	cmd_pa = ((u64)pa_high << 32) | pa_low;
-	cmd_size = crb_fixup_cmd_size(dev, &io_res, cmd_pa,
-				      ioread32(&priv->regs_t->ctrl_cmd_size));
+	cmd_size = ioread32(&priv->regs_t->ctrl_cmd_size);
+
+	cres = crb_containing_resource(&crb_resources, cmd_pa, cmd_size, false);
+	if (cres)
+		cmd_size = crb_fixup_cmd_size(dev, &cres->io_res,
+					      cmd_pa, cmd_size);
 
 	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
 		pa_high, pa_low, cmd_size);
 
-	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
+	priv->cmd = crb_map_res(dev, cres, cmd_pa, cmd_size);
 	if (IS_ERR(priv->cmd)) {
 		ret = PTR_ERR(priv->cmd);
 		goto out;
@@ -566,11 +631,16 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
 	rsp_pa = le64_to_cpu(__rsp_pa);
-	rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
-				      ioread32(&priv->regs_t->ctrl_rsp_size));
+	rsp_size = ioread32(&priv->regs_t->ctrl_rsp_size);
+
+	cres = crb_containing_resource(&crb_resources, rsp_pa,
+				       rsp_size, false);
+	if (cres)
+		rsp_size = crb_fixup_cmd_size(dev, &cres->io_res,
+					      rsp_pa, rsp_size);
 
 	if (cmd_pa != rsp_pa) {
-		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
+		priv->rsp = crb_map_res(dev, cres, rsp_pa, rsp_size);
 		ret = PTR_ERR_OR_ZERO(priv->rsp);
 		goto out;
 	}
@@ -596,6 +666,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	__crb_relinquish_locality(dev, priv, 0);
 
+out_free_crb_resources:
+
+	crb_free_resource_list(&crb_resources);
+
 	return ret;
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11  5:17 [PATCH v2] Fix fTPM on AMD Zen+ CPUs Seunghun Han
2019-09-13 14:00 ` Jarkko Sakkinen
2019-09-13 14:02   ` Jarkko Sakkinen
2019-09-16  5:29     ` Seunghun Han
2019-09-16  9:07       ` Jarkko Sakkinen
2019-09-16  9:43         ` Seunghun Han
  -- strict thread matches above, loose matches on Subject: below --
2019-08-11 18:52 ivan.lazeev
2019-08-12 13:10 ` Jason Gunthorpe
2019-08-12 22:42   ` Vanya Lazeev
2019-08-15 20:47     ` Jarkko Sakkinen

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox