qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Zhao Liu <zhao1.liu@intel.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org, qemu-devel@nongnu.org, qemu-rust@nongnu.org,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 3/3] rust: pl011: Check size of state struct at compile time
Date: Thu, 20 Mar 2025 23:45:30 +0800	[thread overview]
Message-ID: <Z9w4Gttv8QeBRKfZ@intel.com> (raw)
In-Reply-To: <20250320133248.1679485-4-peter.maydell@linaro.org>

> -use std::{ffi::CStr, ptr::addr_of_mut};
> +use std::{ffi::CStr, mem, ptr::addr_of_mut};

maybe mem::size_of (since there're 2 use cases :-))? 

>  
>  use qemu_api::{
> +    bindings,
>      chardev::{CharBackend, Chardev, Event},
> +    static_assert,

This one looks like it breaks the alphabetical ordering (this nit can be
checked and fixed by "cargo +nightly fmt" under rust directory, which is
like checkpatch.pl).

>      impl_vmstate_forward,
>      irq::{IRQState, InterruptSource},
>      memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
> @@ -124,6 +126,12 @@ pub struct PL011State {
>      pub migrate_clock: bool,
>  }
>  
> +// Some C users of this device embed its state struct into their own
> +// structs, so the size of the Rust version must not be any larger
> +// than the size of the C one. If this assert triggers you need to
> +// expand the padding_for_rust[] array in the C PL011State struct.
> +static_assert!(mem::size_of::<PL011State>() <= mem::size_of::<bindings::PL011State>());
> +

maybe use qemu_api::bindings::PL011State directly? Because bindings
contains native C structures/functions and their use should not be
encouraged, I think it's better to 'hide' bindings (not list it at the
beginning of the file).

Otherwise,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>



  reply	other threads:[~2025-03-20 15:27 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-20 13:32 [PATCH 0/3] rust: Fix PL011State size mismatch assert Peter Maydell
2025-03-20 13:32 ` [PATCH 1/3] rust: assertions: add static_assert Peter Maydell
2025-03-20 14:03   ` Philippe Mathieu-Daudé
2025-03-20 15:20   ` Zhao Liu
2025-03-20 13:32 ` [PATCH 2/3] hw/char/pl011: Pad PL011State struct to same size as Rust impl Peter Maydell
2025-03-20 15:28   ` Zhao Liu
2025-03-20 13:32 ` [PATCH 3/3] rust: pl011: Check size of state struct at compile time Peter Maydell
2025-03-20 15:45   ` Zhao Liu [this message]
2025-03-20 16:02     ` Peter Maydell
2025-03-21  4:31       ` Zhao Liu

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=Z9w4Gttv8QeBRKfZ@intel.com \
    --to=zhao1.liu@intel.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-rust@nongnu.org \
    /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).