Re: [PATCH 3/4] Allow tests to be disabled
On Wed, 17 Jan 2024 at 12:59, Manolo de Medici wrote: > > tests/qtest/tpm-* compilation is not disabled by disable-tpm, > for this reason compilation fails on systems that doesn't > support the linux/bsd TPM api. Fix this by allowing tests > to be disabled. This isn't the right way to fix this. Either the tpm test code has portability issues that can be fixed, or else it should be guarded by a suitable check that means we don't try to build it on hosts that don't have the host-specific functions/whatever that it needs. thanks -- PMM
Re: [PATCH v2 0/4] Port qemu to GNU/Hurd
On Thu, 18 Jan 2024 at 16:02, Manolo de Medici wrote: > > Recently, a testsuite for gnumach, the GNU/Hurd microkernel, was developed > that uses qemu. Currently qemu cannot be compiled for the GNU/Hurd, as such, > this testsuite is available only for GNU/Linux users. > > This patcheset allows qemu compilation in GNU/Hurd. With this patchset > applied, > qemu can be compiled without any special configure options. > > Please review, thanks, > > Manolo de Medici (4): > Include new arbitrary limits if not already defined > Avoid conflicting types for 'copy_file_range' > Add the GNU/Hurd as a target host > Exclude TPM ioctls definitions for the GNU/Hurd Hi -- something odd seems to have happened with these patchset emails. The cover letter got to the list: https://lore.kernel.org/qemu-devel/CAHP40m=UQ=F1-Vy4-wR18RjqzF9o+8UOjgpUsrTU8QXn=7e...@mail.gmail.com/ but it doesn't have any of the patches as followup emails in the thread. Instead they got sent as entirely separate emails, eg here's patch 1: https://lore.kernel.org/qemu-devel/cahp40mmk4cpk6zhetfq5btqxk63a6piuckrvv4yyopbxvtw...@mail.gmail.com/ This means the automated patch tooling thinks none of the patches arrived, eg patchew says "0 patches received": https://lore.kernel.org/qemu-devel/cahp40mmk4cpk6zhetfq5btqxk63a6piuckrvv4yyopbxvtw...@mail.gmail.com/ git format-patch can help in getting this right (it is its "--thread=shallow" style). For this version, I'll send some review comments to the individual patch emails, but it would be nice to get this right on v3. thanks -- PMM
Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
(Cc'ing the block folks)
On Thu, 18 Jan 2024 at 16:03, Manolo de Medici wrote:
>
> Compilation fails on systems where copy_file_range is already defined as a
> stub.
What do you mean by "stub" here ? If the system headers define
a prototype for the function, I would have expected the
meson check to set HAVE_COPY_FILE_RANGE, and then this
function doesn't get defined at all. That is, the prototype
mismatch shouldn't matter because if the prototype exists we
use the libc function, and if it doesn't then we use our version.
> The prototype of copy_file_range in glibc returns an ssize_t, not an off_t.
>
> The function currently only exists on linux and freebsd, and in both cases
> the return type is ssize_t
>
> Signed-off-by: Manolo de Medici
> ---
> block/file-posix.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..f744b35642 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2000,12 +2000,13 @@ static int handle_aiocb_write_zeroes_unmap(void
> *opaque)
> }
>
> #ifndef HAVE_COPY_FILE_RANGE
> -static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
> - off_t *out_off, size_t len, unsigned int flags)
> +ssize_t copy_file_range (int infd, off_t *pinoff,
> + int outfd, off_t *poutoff,
> + size_t length, unsigned int flags)
No space after "copy_file_range". No need to rename all the
arguments either.
> {
> #ifdef __NR_copy_file_range
> -return syscall(__NR_copy_file_range, in_fd, in_off, out_fd,
> - out_off, len, flags);
> +return (ssize_t)syscall(__NR_copy_file_range, infd, pinoff, outfd,
> +poutoff, length, flags);
Don't need a cast here, because returning the value will
automatically cast it to the right thing.
> #else
> errno = ENOSYS;
> return -1;
These changes aren't wrong, but as noted above I'm surprised that
the Hurd gets into this code at all.
Note for Kevin: shouldn't this direct use of syscall() have
some sort of OS-specific guard on it? There's nothing that
says that a non-Linux host OS has to have the same set of
arguments to an __NR_copy_file_range syscall. If this
fallback is a Linux-ism we should guard it appropriately.
For that matter, at what point can we just remove the fallback
entirely? copy_file_range() went into Linux in 4.5, apparently,
which is now nearly 8 years old. Maybe all our supported
hosts now have a new enough kernel and we can drop this
somewhat ugly syscall() wrapper...
thanks
-- PMM
Re: [PATCH v2 3/4] Add the GNU/Hurd as a target host
On Thu, 18 Jan 2024 at 16:04, Manolo de Medici wrote: > > Signed-off-by: Manolo de Medici > --- > configure | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/configure b/configure > index 21ab9a64e9..fb11ede5b2 100755 > --- a/configure > +++ b/configure > @@ -353,6 +353,8 @@ elif check_define __NetBSD__; then >host_os=netbsd > elif check_define __APPLE__; then >host_os=darwin > +elif check_define __GNU__; then > + host_os=hurd > else Meson's official value for the OS name for GNU Hurd is "gnu": https://mesonbuild.com/Reference-tables.html#operating-system-names so we should use the same string here in configure too. thanks -- PMM
Re: [PATCH v2 4/4] Exclude TPM ioctls definitions for the GNU/Hurd
On Thu, 18 Jan 2024 at 16:04, Manolo de Medici wrote:
>
> The Hurd currently doesn't have any TPM driver, compilation fails
> for missing symbols unless these are left undefined.
>
> Signed-off-by: Manolo de Medici
> ---
> backends/tpm/tpm_ioctl.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/backends/tpm/tpm_ioctl.h b/backends/tpm/tpm_ioctl.h
> index 1933ab6855..c721bf8847 100644
> --- a/backends/tpm/tpm_ioctl.h
> +++ b/backends/tpm/tpm_ioctl.h
> @@ -274,7 +274,7 @@ typedef struct ptm_lockstorage ptm_lockstorage;
> #define PTM_CAP_SEND_COMMAND_HEADER (1 << 15)
> #define PTM_CAP_LOCK_STORAGE (1 << 16)
>
> -#ifndef _WIN32
> +#if !defined(_WIN32) && !defined(__GNU__)
> enum {
> PTM_GET_CAPABILITY = _IOR('P', 0, ptm_cap),
> PTM_INIT = _IOWR('P', 1, ptm_init),
> --
> 2.43.0
This looks plausible as a change, but looking at the history
of the file in git it seems like this is a file we import
from a third-party swtpm project.
Stefan: should we get this change made in the swtpm project
too? Or have we diverged from that copy of the header?
If the latter, then the simple thing would be to delete
this enum entirely, because as far as I can see we don't
use any of the values in QEMU, so we can avoid the
portability problem that way.
thanks
-- PMM
Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined
On Thu, 18 Jan 2024 at 16:03, Manolo de Medici wrote:
>
> qemu uses the PATH_MAX and IOV_MAX constants extensively
> in the code. Define these constants to sensible values ourselves
> if the system doesn't define them already.
>
> Signed-off-by: Manolo de Medici
> ---
> include/qemu/osdep.h | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 9a405bed89..9fb6ac5c64 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
> #define TIME_MAX TYPE_MAXIMUM(time_t)
> #endif
>
> +#ifndef PATH_MAX
> +#define PATH_MAX 1024
> +#endif
> +
> +#ifndef IOV_MAX
> +#define IOV_MAX 1024
> +#endif
> +
> /* Mac OSX has a bug that incorrectly defines SIZE_MAX with
> * the wrong type. Our replacement isn't usable in preprocessor
> * expressions, but it is sufficient for our needs. */
Ccing some people who know more about portability concerns
than I do...
thanks
-- PMM
Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined
On Mon, 22 Jan 2024 at 17:27, Daniel P. Berrangé wrote:
>
> On Thu, Jan 18, 2024 at 05:02:23PM +0100, Manolo de Medici wrote:
> > qemu uses the PATH_MAX and IOV_MAX constants extensively
> > in the code. Define these constants to sensible values ourselves
> > if the system doesn't define them already.
>
> Please give details of what platform(s) lack these constants
> in the commit message.
>
> Presumably this is a platform that is outside of our normal
> support build target list, since we have at least build
> coverage for everything mainstream.
It's GNU Hurd. The patchset isn't threaded, but the cover
letter is
https://lore.kernel.org/qemu-devel/CAHP40m=UQ=F1-Vy4-wR18RjqzF9o+8UOjgpUsrTU8QXn=7e...@mail.gmail.com/
and you can pick up the other patches in it by searching the list.
> >
> > Signed-off-by: Manolo de Medici
> > ---
> > include/qemu/osdep.h | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 9a405bed89..9fb6ac5c64 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
> > #define TIME_MAX TYPE_MAXIMUM(time_t)
> > #endif
> >
> > +#ifndef PATH_MAX
> > +#define PATH_MAX 1024
> > +#endif
> > +
> > +#ifndef IOV_MAX
> > +#define IOV_MAX 1024
> > +#endif
>
> If we're going to add this, since we should be removing the
> later duplication:
>
> #define IOV_MAX 1024
>
> in this same file
Mmm, I wondered about that, although in that case it's
"for when the host has no iov implementation at all
and we're rolling our own".
thanks
-- PMM
Re: [PATCH v2 4/4] Exclude TPM ioctls definitions for the GNU/Hurd
On Mon, 22 Jan 2024 at 19:30, Stefan Berger wrote:
>
>
>
> On 1/22/24 12:16, Peter Maydell wrote:
> > On Thu, 18 Jan 2024 at 16:04, Manolo de Medici
> > wrote:
> >>
> >> The Hurd currently doesn't have any TPM driver, compilation fails
> >> for missing symbols unless these are left undefined.
> >>
> >> Signed-off-by: Manolo de Medici
> >> ---
> >> backends/tpm/tpm_ioctl.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/backends/tpm/tpm_ioctl.h b/backends/tpm/tpm_ioctl.h
> >> index 1933ab6855..c721bf8847 100644
> >> --- a/backends/tpm/tpm_ioctl.h
> >> +++ b/backends/tpm/tpm_ioctl.h
> >> @@ -274,7 +274,7 @@ typedef struct ptm_lockstorage ptm_lockstorage;
> >> #define PTM_CAP_SEND_COMMAND_HEADER (1 << 15)
> >> #define PTM_CAP_LOCK_STORAGE (1 << 16)
> >>
> >> -#ifndef _WIN32
> >> +#if !defined(_WIN32) && !defined(__GNU__)
> >> enum {
> >> PTM_GET_CAPABILITY = _IOR('P', 0, ptm_cap),
> >> PTM_INIT = _IOWR('P', 1, ptm_init),
> >> --
> >> 2.43.0
> >
> > This looks plausible as a change, but looking at the history
> > of the file in git it seems like this is a file we import
> > from a third-party swtpm project.
> >
> > Stefan: should we get this change made in the swtpm project
> > too? Or have we diverged from that copy of the header?
>
> The diffs are minimal at the moment:
> $ diff swtpm/include/swtpm/tpm_ioctl.h qemu/backends/tpm/tpm_ioctl.h
> 15,16d14
> < #include
> < #include
>
> Since we already handle _WIN32 we can just take this case for __GNU__.
OK, so how should we handle the mechanics of it -- just take
this commit in QEMU and then you'll sort it out in swtpm?
Or do we need to change swtpm first and then sync?
thanks
-- PMM
