qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Auger <eric.auger@redhat.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: stefanb@linux.vnet.ibm.com, cohuck@redhat.com,
	qemu-devel@nongnu.org, f4bug@amsat.org, quintela@redhat.com,
	alex.williamson@redhat.com, imammedo@redhat.com,
	eric.auger.pro@gmail.com, david@gibson.dropbear.id.au
Subject: Re: [PATCH v4 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region
Date: Wed, 9 Feb 2022 10:39:40 +0100	[thread overview]
Message-ID: <eae7e6e6-2f56-c263-f1d2-19104201c8ec@redhat.com> (raw)
In-Reply-To: <YgKifeZwaWRZ1V1l@work-vm>

Hi Dave,

On 2/8/22 6:03 PM, Dr. David Alan Gilbert wrote:
> * Eric Auger (eric.auger@redhat.com) wrote:
>> Representing the CRB cmd/response buffer as a standard
>> RAM region causes some trouble when the device is used
>> with VFIO. Indeed VFIO attempts to DMA_MAP this region
>> as usual RAM but this latter does not have a valid page
>> size alignment causing such an error report:
>> "vfio_listener_region_add received unaligned region".
>> To allow VFIO to detect that failing dma mapping
>> this region is not an issue, let's use a ram_device
>> memory region type instead.
> Don't the weird sizes and alignments also cause problems with the RAM
> migration code?
I tested CRB migration and it seems to work properly.
> Why don't you just align this up to a convenient boundary?
The spec (CG PC Client Platform TPM Profile (PTP)
    Specification Family “2.0” Level 00 Revision 01.03 v22, page 100) 
says that the command/response data "may be defined as large as 3968",
which is (0x1000 - 0x80), 0x80 being the size of the control struct.
so the size of the region logically is less than a 4kB page, hence our
trouble.

We learnt in the past Windows driver has some stronger expectation wrt
memory mapping. I don't know if those latter would work if we were to
enlarge the window by some tricks.


Besides
https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf
says

"
Including the control structure, the three memory areas comprise the
entirety of the CRB. There are no constraints on how those three memory
areas are provided. They can all be in system RAM, or all be in device
memory, or any combination.
"
So device memory looks an option too.

Adding Marc-Andre in the loop

Thanks

Eric





>
> Dave
>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
>> Acked-by: Stefan Berger <stefanb@linux.ibm.com>
>> [PMD: Keep tpm_crb.c in meson's softmmu_ss]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> ---
>>
>> v3 -> v4:
>> -  call vmstate_unregister_ram
>> ---
>>  hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>> index 58ebd1469c3..668f969b409 100644
>> --- a/hw/tpm/tpm_crb.c
>> +++ b/hw/tpm/tpm_crb.c
>> @@ -25,6 +25,7 @@
>>  #include "sysemu/tpm_backend.h"
>>  #include "sysemu/tpm_util.h"
>>  #include "sysemu/reset.h"
>> +#include "exec/cpu-common.h"
>>  #include "tpm_prop.h"
>>  #include "tpm_ppi.h"
>>  #include "trace.h"
>> @@ -43,6 +44,7 @@ struct CRBState {
>>  
>>      bool ppi_enabled;
>>      TPMPPI ppi;
>> +    uint8_t *crb_cmd_buf;
>>  };
>>  typedef struct CRBState CRBState;
>>  
>> @@ -291,10 +293,14 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>>          return;
>>      }
>>  
>> +    s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size,
>> +                                HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE));
>> +
>>      memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s,
>>          "tpm-crb-mmio", sizeof(s->regs));
>> -    memory_region_init_ram(&s->cmdmem, OBJECT(s),
>> -        "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
>> +    memory_region_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd",
>> +                                      CRB_CTRL_CMD_SIZE, s->crb_cmd_buf);
>> +    vmstate_register_ram(&s->cmdmem, dev);
>>  
>>      memory_region_add_subregion(get_system_memory(),
>>          TPM_CRB_ADDR_BASE, &s->mmio);
>> @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>>      qemu_register_reset(tpm_crb_reset, dev);
>>  }
>>  
>> +static void tpm_crb_unrealize(DeviceState *dev)
>> +{
>> +    CRBState *s = CRB(dev);
>> +
>> +    vmstate_unregister_ram(&s->cmdmem, dev);
>> +    qemu_vfree(s->crb_cmd_buf);
>> +
>> +    if (s->ppi_enabled) {
>> +        qemu_vfree(s->ppi.buf);
>> +    }
>> +}
>> +
>>  static void tpm_crb_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>      TPMIfClass *tc = TPM_IF_CLASS(klass);
>>  
>>      dc->realize = tpm_crb_realize;
>> +    dc->unrealize = tpm_crb_unrealize;
>>      device_class_set_props(dc, tpm_crb_properties);
>>      dc->vmsd  = &vmstate_tpm_crb;
>>      dc->user_creatable = true;
>> -- 
>> 2.26.3
>>



  reply	other threads:[~2022-02-09 10:16 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-08 13:38 [PATCH v4 0/2] TPM-CRB: Remove spurious error report when used with VFIO Eric Auger
2022-02-08 13:38 ` [PATCH v4 1/2] tpm: CRB: Use ram_device for "tpm-crb-cmd" region Eric Auger
2022-02-08 15:17   ` Peter Maydell
2022-02-08 15:56     ` Eric Auger
2022-02-08 16:01       ` Peter Maydell
2022-02-08 16:36         ` Alex Williamson
2022-02-08 17:14           ` Peter Maydell
2022-02-09  9:54             ` Eric Auger
2022-02-08 16:42         ` Eric Auger
2022-02-08 17:03   ` Dr. David Alan Gilbert
2022-02-09  9:39     ` Eric Auger [this message]
2022-02-08 17:16   ` Stefan Berger
2022-02-08 17:58     ` Eric Auger
2022-03-03 14:37       ` Eric Auger
2022-03-03 16:16         ` Marc-André Lureau
2022-03-04  9:32           ` Eric Auger
2022-02-08 13:38 ` [PATCH v4 2/2] hw/vfio/common: Silence ram device offset alignment error traces Eric Auger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eae7e6e6-2f56-c263-f1d2-19104201c8ec@redhat.com \
    --to=eric.auger@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=cohuck@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=dgilbert@redhat.com \
    --cc=eric.auger.pro@gmail.com \
    --cc=f4bug@amsat.org \
    --cc=imammedo@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=stefanb@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).