Re: [Virtio-fs] [edk2 PATCH 06/48] OvmfPkg/VirtioFsDxe: introduce the basic FUSE request/response headers

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 12/17/20 12:49, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (lersek@redhat.com) wrote:
> >> Introduce the VIRTIO_FS_FUSE_REQUEST and VIRTIO_FS_FUSE_RESPONSE
> >> structures, which are the common headers for the various FUSE
> >> request/response structures.
> >>
> >> Introduce the VirtioFsFuseNewRequest() helper function for populating
> >> VIRTIO_FS_FUSE_REQUEST, from parameters and from a VIRTIO_FS-level request
> >> counter.
> >>
> >> Introduce the VirtioFsFuseCheckResponse() helper function for verifying
> >> most FUSE response types that begin with the VIRTIO_FS_FUSE_RESPONSE
> >> header.
> >>
> >> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3097
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>  OvmfPkg/Include/IndustryStandard/VirtioFs.h |  49 +++++
> >>  OvmfPkg/VirtioFsDxe/VirtioFsDxe.h           |  17 ++
> >>  OvmfPkg/VirtioFsDxe/DriverBinding.c         |   5 +
> >>  OvmfPkg/VirtioFsDxe/Helpers.c               | 216 ++++++++++++++++++++
> >>  4 files changed, 287 insertions(+)
> >>
> >> diff --git a/OvmfPkg/Include/IndustryStandard/VirtioFs.h b/OvmfPkg/Include/IndustryStandard/VirtioFs.h
> >> index ea7d80d15d0b..521288b03f1c 100644
> >> --- a/OvmfPkg/Include/IndustryStandard/VirtioFs.h
> >> +++ b/OvmfPkg/Include/IndustryStandard/VirtioFs.h
> >> @@ -44,9 +44,58 @@ typedef struct {
> >>    //
> >>    // The total number of request virtqueues exposed by the device (i.e.,
> >>    // excluding the "hiprio" queue).
> >>    //
> >>    UINT32 NumReqQueues;
> >>  } VIRTIO_FS_CONFIG;
> >>  #pragma pack ()
> >>  
> >> +//
> >> +// FUSE-related definitions follow.
> >> +//
> >> +// From virtio-v1.1-cs01-87fa6b5d8155, 5.11 File System Device: "[...] The
> >> +// driver acts as the FUSE client mounting the file system. The virtio file
> >> +// system device provides the mechanism for transporting FUSE requests [...]"
> >> +//
> >> +// Unfortunately, the documentation of the FUSE wire protocol is lacking. The
> >> +// Virtio spec (as of this writing) simply defers to
> >> +// "include/uapi/linux/fuse.h" in the Linux kernel source -- see the reference
> >> +// in virtio spec file "introduction.tex", at commit 87fa6b5d8155.
> >> +//
> >> +// Of course, "include/uapi/linux/fuse.h" is a moving target (the virtio spec
> >> +// does not specify a particular FUSE interface version). The OvmfPkg code
> >> +// targets version 7.31, because that's the lowest version that the QEMU
> >> +// virtio-fs daemon supports at this time -- see QEMU commit 72c42e2d6551
> >> +// ("virtiofsd: Trim out compatibility code", 2020-01-23).
> > 
> > Fuse has it's own in built protocol versioning and negotiation, and as I
> > remember it's the version of the header in the Linux kernel that's the
> > definitive version.  I think it would have also been safe just to take
> > the latest released kernel version
> 
> 7.32 (kernel commit c6ff213fe5b8) introduces "submount support", which
> does not seem relevant. The strategy we follow in
> "OvmfPkg/Include/IndustryStandard" is: never include or convert header
> files from other projects whole-sale; always evaluate every structure
> and macro one by one for necessity, and if a definition is really needed
> for the driver being implemented, add that definition manually, and
> natively to the edk2 coding style.

The 'submount support' is probably something you don't need; note that
you may end up with multiple files with the same inode number
(if the host fs has submounts).

> This approach has caused zero friction or extra maintenance work over
> the years, and it allows for a native look & feel. (It's a different
> question what people think of edk2's "native" look and feel :))
> 
> A counter-example is the Xen headers in
> "OvmfPkg/Include/IndustryStandard/Xen", which were mass-imported and
> then fixed up separately for edk2 compatibility at some point -- much to
> my dismay. They feel totally foreign in edk2 and they've never stopped
> being a thorn in my side. It's not just the headers of course: when you
> *use* those macros and typedefs in the actual driver code, it shows.
> 
> So, as long as it's up to me, OVMF never "just takes" but "adds from
> zero", and I do stick with the absolute minimum needed.

Yep that's fine.

Dave

> Thanks,
> Laszlo
> 
> > 
> > Dave
> > 
> >> +// Correspondingly, Linux's "include/uapi/linux/fuse.h" is consulted as checked
> >> +// out at commit (c6ff213fe5b8^) = d78092e4937d ("fuse: fix page dereference
> >> +// after free", 2020-09-18); that is, right before commit c6ff213fe5b8 ("fuse:
> >> +// add submount support to <uapi/linux/fuse.h>", 2020-09-18) introduces FUSE
> >> +// interface version 7.32.
> >> +//
> >> +#define VIRTIO_FS_FUSE_MAJOR  7
> >> +#define VIRTIO_FS_FUSE_MINOR 31
> >> +
> >> +#pragma pack (1)
> >> +//
> >> +// Request-response headers common to all request types.
> >> +//
> >> +typedef struct {
> >> +  UINT32 Len;
> >> +  UINT32 Opcode;
> >> +  UINT64 Unique;
> >> +  UINT64 NodeId;
> >> +  UINT32 Uid;
> >> +  UINT32 Gid;
> >> +  UINT32 Pid;
> >> +  UINT32 Padding;
> >> +} VIRTIO_FS_FUSE_REQUEST;
> >> +
> >> +typedef struct {
> >> +  UINT32 Len;
> >> +  INT32  Error;
> >> +  UINT64 Unique;
> >> +} VIRTIO_FS_FUSE_RESPONSE;
> >> +#pragma pack ()
> >> +
> >>  #endif // VIRTIO_FS_H_
> >> diff --git a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
> >> index 12acbd6dc359..f7eae9a4b71a 100644
> >> --- a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
> >> +++ b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
> >> @@ -39,16 +39,17 @@ typedef struct {
> >>    //                              field         init function       init depth
> >>    //                              -----------   ------------------  ----------
> >>    UINT64                          Signature; // DriverBindingStart  0
> >>    VIRTIO_DEVICE_PROTOCOL          *Virtio;   // DriverBindingStart  0
> >>    VIRTIO_FS_LABEL                 Label;     // VirtioFsInit        1
> >>    UINT16                          QueueSize; // VirtioFsInit        1
> >>    VRING                           Ring;      // VirtioRingInit      2
> >>    VOID                            *RingMap;  // VirtioRingMap       2
> >> +  UINT64                          RequestId; // DriverBindingStart  0
> >>    EFI_EVENT                       ExitBoot;  // DriverBindingStart  0
> >>    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL SimpleFs;  // DriverBindingStart  0
> >>  } VIRTIO_FS;
> >>  
> >>  #define VIRTIO_FS_FROM_SIMPLE_FS(SimpleFsReference) \
> >>    CR (SimpleFsReference, VIRTIO_FS, SimpleFs, VIRTIO_FS_SIG);
> >>  
> >>  //
> >> @@ -127,16 +128,32 @@ VirtioFsSgListsValidate (
> >>  
> >>  EFI_STATUS
> >>  VirtioFsSgListsSubmit (
> >>    IN OUT VIRTIO_FS                     *VirtioFs,
> >>    IN OUT VIRTIO_FS_SCATTER_GATHER_LIST *RequestSgList,
> >>    IN OUT VIRTIO_FS_SCATTER_GATHER_LIST *ResponseSgList OPTIONAL
> >>    );
> >>  
> >> +EFI_STATUS
> >> +VirtioFsFuseNewRequest (
> >> +  IN OUT VIRTIO_FS              *VirtioFs,
> >> +     OUT VIRTIO_FS_FUSE_REQUEST *Request,
> >> +  IN     UINT32                 RequestSize,
> >> +  IN     UINT32                 Opcode,
> >> +  IN     UINT64                 NodeId
> >> +  );
> >> +
> >> +EFI_STATUS
> >> +VirtioFsFuseCheckResponse (
> >> +  IN  VIRTIO_FS_SCATTER_GATHER_LIST *ResponseSgList,
> >> +  IN  UINT64                        RequestId,
> >> +  OUT UINTN                         *TailBufferFill
> >> +  );
> >> +
> >>  //
> >>  // EFI_SIMPLE_FILE_SYSTEM_PROTOCOL member functions for the Virtio Filesystem
> >>  // driver.
> >>  //
> >>  
> >>  EFI_STATUS
> >>  EFIAPI
> >>  VirtioFsOpenVolume (
> >> diff --git a/OvmfPkg/VirtioFsDxe/DriverBinding.c b/OvmfPkg/VirtioFsDxe/DriverBinding.c
> >> index b888158a805d..4a2787a50a6e 100644
> >> --- a/OvmfPkg/VirtioFsDxe/DriverBinding.c
> >> +++ b/OvmfPkg/VirtioFsDxe/DriverBinding.c
> >> @@ -79,16 +79,21 @@ VirtioFsBindingStart (
> >>      goto FreeVirtioFs;
> >>    }
> >>  
> >>    Status = VirtioFsInit (VirtioFs);
> >>    if (EFI_ERROR (Status)) {
> >>      goto CloseVirtio;
> >>    }
> >>  
> >> +  //
> >> +  // Initialize the FUSE request counter.
> >> +  //
> >> +  VirtioFs->RequestId = 1;
> >> +
> >>    Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> >>                    VirtioFsExitBoot, VirtioFs, &VirtioFs->ExitBoot);
> >>    if (EFI_ERROR (Status)) {
> >>      goto UninitVirtioFs;
> >>    }
> >>  
> >>    VirtioFs->SimpleFs.Revision   = EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_REVISION;
> >>    VirtioFs->SimpleFs.OpenVolume = VirtioFsOpenVolume;
> >> diff --git a/OvmfPkg/VirtioFsDxe/Helpers.c b/OvmfPkg/VirtioFsDxe/Helpers.c
> >> index 88264d4b264c..5bd2dc641f6d 100644
> >> --- a/OvmfPkg/VirtioFsDxe/Helpers.c
> >> +++ b/OvmfPkg/VirtioFsDxe/Helpers.c
> >> @@ -693,8 +693,224 @@ VirtioFsSgListsSubmit (
> >>        if (!EFI_ERROR (Status) && EFI_ERROR (UnmapStatus)) {
> >>          Status = UnmapStatus;
> >>        }
> >>      }
> >>    }
> >>  
> >>    return Status;
> >>  }
> >> +
> >> +/**
> >> +  Set up the fields of a new VIRTIO_FS_FUSE_REQUEST object.
> >> +
> >> +  The function may only be called after VirtioFsInit() returns successfully and
> >> +  before VirtioFsUninit() is called.
> >> +
> >> +  @param[in,out] VirtioFs The Virtio Filesystem device that the request is
> >> +                          being prepared for. The "VirtioFs->RequestId" field
> >> +                          will be copied into "Request->Unique". On output (on
> >> +                          successful return), "VirtioFs->RequestId" will be
> >> +                          incremented.
> >> +
> >> +  @param[out] Request     The VIRTIO_FS_FUSE_REQUEST object whose fields are to
> >> +                          be set.
> >> +
> >> +  @param[in] RequestSize  The total size of the request, including
> >> +                          sizeof(VIRTIO_FS_FUSE_REQUEST).
> >> +
> >> +  @param[in] Opcode       The VIRTIO_FS_FUSE_OPCODE that identifies the command
> >> +                          to send.
> >> +
> >> +  @param[in] NodeId       The inode number of the file that the request refers
> >> +                          to.
> >> +
> >> +  @retval EFI_INVALID_PARAMETER  RequestSize is smaller than
> >> +                                 sizeof(VIRTIO_FS_FUSE_REQUEST).
> >> +
> >> +  @retval EFI_OUT_OF_RESOURCES   "VirtioFs->RequestId" is MAX_UINT64, and can
> >> +                                 be incremented no more.
> >> +
> >> +  @retval EFI_SUCCESS            Request has been populated,
> >> +                                 "VirtioFs->RequestId" has been incremented.
> >> +**/
> >> +EFI_STATUS
> >> +VirtioFsFuseNewRequest (
> >> +  IN OUT VIRTIO_FS              *VirtioFs,
> >> +     OUT VIRTIO_FS_FUSE_REQUEST *Request,
> >> +  IN     UINT32                 RequestSize,
> >> +  IN     UINT32                 Opcode,
> >> +  IN     UINT64                 NodeId
> >> +  )
> >> +{
> >> +  if (RequestSize < sizeof *Request) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  if (VirtioFs->RequestId == MAX_UINT64) {
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  Request->Len     = RequestSize;
> >> +  Request->Opcode  = Opcode;
> >> +  Request->Unique  = VirtioFs->RequestId++;
> >> +  Request->NodeId  = NodeId;
> >> +  Request->Uid     = 0;
> >> +  Request->Gid     = 0;
> >> +  Request->Pid     = 1;
> >> +  Request->Padding = 0;
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/**
> >> +  Check the common FUSE response format.
> >> +
> >> +  The first buffer in the response scatter-gather list is assumed a
> >> +  VIRTIO_FS_FUSE_RESPONSE structure. Subsequent response buffers, if any, up to
> >> +  and excluding the last one, are assumed fixed size. The last response buffer
> >> +  may or may not be fixed size, as specified by the caller.
> >> +
> >> +  This function may only be called after VirtioFsSgListsSubmit() returns
> >> +  successfully.
> >> +
> >> +  @param[in] ResponseSgList   The scatter-gather list that describes the
> >> +                              response part of the exchange -- the buffers that
> >> +                              the Virtio Filesystem device filled in during the
> >> +                              virtio transfer.
> >> +
> >> +  @param[in] RequestId        The request identifier to which the response is
> >> +                              expected to belong.
> >> +
> >> +  @param[out] TailBufferFill  If NULL, then the last buffer in ResponseSgList
> >> +                              is considered fixed size. Otherwise, the last
> >> +                              buffer is considered variable size, and on
> >> +                              successful return, TailBufferFill reports the
> >> +                              number of bytes in the last buffer.
> >> +
> >> +  @retval EFI_INVALID_PARAMETER  TailBufferFill is not NULL (i.e., the last
> >> +                                 buffer is considered variable size), and
> >> +                                 ResponseSgList->NumVec is 1.
> >> +
> >> +  @retval EFI_INVALID_PARAMETER  The allocated size of the first buffer does
> >> +                                 not match sizeof(VIRTIO_FS_FUSE_RESPONSE).
> >> +
> >> +  @retval EFI_PROTOCOL_ERROR     The VIRTIO_FS_FUSE_RESPONSE structure in the
> >> +                                 first buffer has not been fully populated.
> >> +
> >> +  @retval EFI_PROTOCOL_ERROR     "VIRTIO_FS_FUSE_RESPONSE.Len" in the first
> >> +                                 buffer does not equal the sum of the
> >> +                                 individual buffer sizes (as populated).
> >> +
> >> +  @retval EFI_PROTOCOL_ERROR     "VIRTIO_FS_FUSE_RESPONSE.Unique" in the first
> >> +                                 buffer does not equal RequestId.
> >> +
> >> +  @retval EFI_PROTOCOL_ERROR     "VIRTIO_FS_FUSE_RESPONSE.Error" in the first
> >> +                                 buffer is zero, but a subsequent fixed size
> >> +                                 buffer has not been fully populated.
> >> +
> >> +  @retval EFI_DEVICE_ERROR       "VIRTIO_FS_FUSE_RESPONSE.Error" in the first
> >> +                                 buffer is nonzero. The caller may investigate
> >> +                                 "VIRTIO_FS_FUSE_RESPONSE.Error". Note that the
> >> +                                 completeness of the subsequent fixed size
> >> +                                 buffers is not verified in this case.
> >> +
> >> +  @retval EFI_SUCCESS            Verification successful.
> >> +**/
> >> +EFI_STATUS
> >> +VirtioFsFuseCheckResponse (
> >> +  IN  VIRTIO_FS_SCATTER_GATHER_LIST *ResponseSgList,
> >> +  IN  UINT64                        RequestId,
> >> +  OUT UINTN                         *TailBufferFill
> >> +  )
> >> +{
> >> +  UINTN                   NumFixedSizeVec;
> >> +  VIRTIO_FS_FUSE_RESPONSE *CommonResp;
> >> +  UINT32                  TotalTransferred;
> >> +  UINTN                   Idx;
> >> +
> >> +  //
> >> +  // Ensured by VirtioFsSgListsValidate().
> >> +  //
> >> +  ASSERT (ResponseSgList->NumVec > 0);
> >> +
> >> +  if (TailBufferFill == NULL) {
> >> +    //
> >> +    // All buffers are considered fixed size.
> >> +    //
> >> +    NumFixedSizeVec = ResponseSgList->NumVec;
> >> +  } else {
> >> +    //
> >> +    // If the last buffer is variable size, then we need that buffer to be
> >> +    // different from the first buffer, which is considered a
> >> +    // VIRTIO_FS_FUSE_RESPONSE (fixed size) structure.
> >> +    //
> >> +    if (ResponseSgList->NumVec == 1) {
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> >> +    NumFixedSizeVec = ResponseSgList->NumVec - 1;
> >> +  }
> >> +
> >> +  //
> >> +  // The first buffer is supposed to carry a (fully populated)
> >> +  // VIRTIO_FS_FUSE_RESPONSE structure.
> >> +  //
> >> +  if (ResponseSgList->IoVec[0].Size != sizeof *CommonResp) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  if (ResponseSgList->IoVec[0].Transferred != ResponseSgList->IoVec[0].Size) {
> >> +    return EFI_PROTOCOL_ERROR;
> >> +  }
> >> +
> >> +  //
> >> +  // FUSE must report the same number of bytes, written by the Virtio
> >> +  // Filesystem device, as the virtio transport does.
> >> +  //
> >> +  CommonResp = ResponseSgList->IoVec[0].Buffer;
> >> +  TotalTransferred = 0;
> >> +  for (Idx = 0; Idx < ResponseSgList->NumVec; Idx++) {
> >> +    //
> >> +    // Integer overflow and truncation are not possible, based on
> >> +    // VirtioFsSgListsValidate() and VirtioFsSgListsSubmit().
> >> +    //
> >> +    TotalTransferred += (UINT32)ResponseSgList->IoVec[Idx].Transferred;
> >> +  }
> >> +  if (CommonResp->Len != TotalTransferred) {
> >> +    return EFI_PROTOCOL_ERROR;
> >> +  }
> >> +
> >> +  //
> >> +  // Enforce that FUSE match our request ID in the response.
> >> +  //
> >> +  if (CommonResp->Unique != RequestId) {
> >> +    return EFI_PROTOCOL_ERROR;
> >> +  }
> >> +
> >> +  //
> >> +  // If there is an explicit error report, skip checking the transfer
> >> +  // counts for the rest of the fixed size buffers.
> >> +  //
> >> +  if (CommonResp->Error != 0) {
> >> +    return EFI_DEVICE_ERROR;
> >> +  }
> >> +
> >> +  //
> >> +  // There was no error reported, so we require that the Virtio Filesystem
> >> +  // device populate all fixed size buffers. We checked this for the very first
> >> +  // buffer above; let's check the rest (if any).
> >> +  //
> >> +  ASSERT (NumFixedSizeVec >= 1);
> >> +  for (Idx = 1; Idx < NumFixedSizeVec; Idx++) {
> >> +    if (ResponseSgList->IoVec[Idx].Transferred !=
> >> +        ResponseSgList->IoVec[Idx].Size) {
> >> +      return EFI_PROTOCOL_ERROR;
> >> +    }
> >> +  }
> >> +
> >> +  //
> >> +  // If the last buffer is considered variable size, report its filled size.
> >> +  //
> >> +  if (TailBufferFill != NULL) {
> >> +    *TailBufferFill = ResponseSgList->IoVec[NumFixedSizeVec].Transferred;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> -- 
> >> 2.19.1.3.g30247aa5d201
> >>
> >>
> >>
> >> _______________________________________________
> >> Virtio-fs mailing list
> >> Virtio-fs@redhat.com
> >> https://www.redhat.com/mailman/listinfo/virtio-fs
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK