diff mbox series

[v2,07/39] xen/riscv: introduce arch-riscv/hvm/save.h

Message ID acb870b980a791d7800d47c08c9574275159df39.1700761381.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series Enable build of full Xen for RISC-V | expand

Commit Message

Oleksii Kurochko Nov. 24, 2023, 10:30 a.m. UTC 
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - remove copyright an the top of hvm/save.h as the header write now is a newly
   introduced empty header.
---
 xen/include/public/arch-riscv/hvm/save.h | 20 ++++++++++++++++++++
 xen/include/public/hvm/save.h            |  2 ++
 2 files changed, 22 insertions(+)
 create mode 100644 xen/include/public/arch-riscv/hvm/save.h

Comments

Jan Beulich Dec. 5, 2023, 3:59 p.m. UTC | #1 
On 24.11.2023 11:30, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/public/arch-riscv/hvm/save.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Structure definitions for HVM state that is held by Xen and must
> + * be saved along with the domain's memory and device-model state.
> + */
> +
> +#ifndef __XEN_PUBLIC_HVM_SAVE_RISCV_H__
> +#define __XEN_PUBLIC_HVM_SAVE_RISCV_H__
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Seeing that Arm's is as empty, I wonder why we have it. Julien, Stefano?

> --- a/xen/include/public/hvm/save.h
> +++ b/xen/include/public/hvm/save.h
> @@ -91,6 +91,8 @@ DECLARE_HVM_SAVE_TYPE(END, 0, struct hvm_save_end);
>  #include "../arch-arm/hvm/save.h"
>  #elif defined(__powerpc64__)
>  #include "../arch-ppc.h"
> +#elif defined(__riscv)
> +#include "../arch-riscv/hvm/save.h"
>  #else
>  #error "unsupported architecture"
>  #endif

The PPC part here looks bogus altogether. Shawn?

Jan
Shawn Anastasio Dec. 7, 2023, 6:09 p.m. UTC | #2 
On 12/5/23 9:59 AM, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
>> --- a/xen/include/public/hvm/save.h
>> +++ b/xen/include/public/hvm/save.h
>> @@ -91,6 +91,8 @@ DECLARE_HVM_SAVE_TYPE(END, 0, struct hvm_save_end);
>>  #include "../arch-arm/hvm/save.h"
>>  #elif defined(__powerpc64__)
>>  #include "../arch-ppc.h"
>> +#elif defined(__riscv)
>> +#include "../arch-riscv/hvm/save.h"
>>  #else
>>  #error "unsupported architecture"
>>  #endif
> 
> The PPC part here looks bogus altogether. Shawn?
>

I think my original intention here was to avoid creating yet another
empty header while still having a place to put PPC-specific definitions
that might be required.

See as how the ARM file is entirely empty though, I doubt we'll be any
different, so this could definitely be dropped.

> Jan

Thanks,
Shawn
Oleksii Kurochko Dec. 20, 2023, 8:05 p.m. UTC | #3 
On Tue, 2023-12-05 at 16:59 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/public/arch-riscv/hvm/save.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Structure definitions for HVM state that is held by Xen and
> > must
> > + * be saved along with the domain's memory and device-model state.
> > + */
> > +
> > +#ifndef __XEN_PUBLIC_HVM_SAVE_RISCV_H__
> > +#define __XEN_PUBLIC_HVM_SAVE_RISCV_H__
> > +
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 
> Seeing that Arm's is as empty, I wonder why we have it. Julien,
> Stefano?
It seems to exist to satisfy the 'install-tools-public-headers' target:
install: cannot stat 'xen/arch-arm/hvm/*.h': No such file or directory
Makefile:58: recipe for target 'install' failed
make[1]: *** [install] Error 1
make[1]: Leaving directory '/builds/xen-
project/people/olkur/xen/tools/include'
Makefile:44: recipe for target 'install-tools-public-headers' failed

From tools/include/Makefile:
install: all
...
$(DESTDIR)$(includedir)/xen/arch-arm
	$(INSTALL_DATA) xen/arch-arm/hvm/*.h
$(DESTDIR)$(includedir)/xen/arch-arm/hvm
...

We have the following options:
1. Remove the line with $(INSTALL_DATA) xen/arch-arm/hvm/*.h (only
save.h is now in this folder, which is empty).
2. Don't touch the Arm part, but for PPC and RISC-V, do the following:
#if defined(__i386__) || defined(__x86_64__)
#include "../arch-x86/hvm/save.h"
#elif defined(__arm__) || defined(__aarch64__)
#include "../arch-arm/hvm/save.h"
+#elif defined(__powerpc64__) || defined(__riscv)
+/* no specific header to include */
#else
#error "unsupported architecture"
#endif

3. Provide an asm-generic version of save.h for Arm, PPC, and RISC-V
and use it in public/save.h.

In your opinion, which option would be better?

~ Oleksii
> ....
> > --- a/xen/include/public/hvm/save.h
> > +++ b/xen/include/public/hvm/save.h
> > @@ -91,6 +91,8 @@ DECLARE_HVM_SAVE_TYPE(END, 0, struct
> > hvm_save_end);
> >  #include "../arch-arm/hvm/save.h"
> >  #elif defined(__powerpc64__)
> >  #include "../arch-ppc.h"
> > +#elif defined(__riscv)
> > +#include "../arch-riscv/hvm/save.h"
> >  #else
> >  #error "unsupported architecture"
> >  #endif
> 
> The PPC part here looks bogus altogether. Shawn?
> 
> Jan
Jan Beulich Dec. 21, 2023, 7:58 a.m. UTC | #4 
On 20.12.2023 21:05, Oleksii wrote:
> On Tue, 2023-12-05 at 16:59 +0100, Jan Beulich wrote:
>> On 24.11.2023 11:30, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/include/public/arch-riscv/hvm/save.h
>>> @@ -0,0 +1,20 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Structure definitions for HVM state that is held by Xen and
>>> must
>>> + * be saved along with the domain's memory and device-model state.
>>> + */
>>> +
>>> +#ifndef __XEN_PUBLIC_HVM_SAVE_RISCV_H__
>>> +#define __XEN_PUBLIC_HVM_SAVE_RISCV_H__
>>> +
>>> +#endif
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * tab-width: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>
>> Seeing that Arm's is as empty, I wonder why we have it. Julien,
>> Stefano?

I'm still curious about the reason here, but ...

> It seems to exist to satisfy the 'install-tools-public-headers' target:
> install: cannot stat 'xen/arch-arm/hvm/*.h': No such file or directory
> Makefile:58: recipe for target 'install' failed
> make[1]: *** [install] Error 1
> make[1]: Leaving directory '/builds/xen-
> project/people/olkur/xen/tools/include'
> Makefile:44: recipe for target 'install-tools-public-headers' failed
> 
> From tools/include/Makefile:
> install: all
> ...
> $(DESTDIR)$(includedir)/xen/arch-arm
> 	$(INSTALL_DATA) xen/arch-arm/hvm/*.h
> $(DESTDIR)$(includedir)/xen/arch-arm/hvm
> ...
> 
> We have the following options:
> 1. Remove the line with $(INSTALL_DATA) xen/arch-arm/hvm/*.h (only
> save.h is now in this folder, which is empty).

... we can't easily remove any existing public header. We can only try to
avoid making the same mistake (even if it's just a minor one) again.

> 2. Don't touch the Arm part, but for PPC and RISC-V, do the following:
> #if defined(__i386__) || defined(__x86_64__)
> #include "../arch-x86/hvm/save.h"
> #elif defined(__arm__) || defined(__aarch64__)
> #include "../arch-arm/hvm/save.h"
> +#elif defined(__powerpc64__) || defined(__riscv)
> +/* no specific header to include */
> #else
> #error "unsupported architecture"
> #endif

Yes. Still awaiting Shawn's input here as well, though.

> 3. Provide an asm-generic version of save.h for Arm, PPC, and RISC-V
> and use it in public/save.h.

That's not an option imo - what's under public/ needs to be self-contained.
Stuff there isn't supposed to even know of asm-generic/.

Jan
Oleksii Kurochko Dec. 21, 2023, 9:42 a.m. UTC | #5 
On Thu, 2023-12-21 at 08:58 +0100, Jan Beulich wrote:
> On 20.12.2023 21:05, Oleksii wrote:
> > On Tue, 2023-12-05 at 16:59 +0100, Jan Beulich wrote:
> > > On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/include/public/arch-riscv/hvm/save.h
> > > > @@ -0,0 +1,20 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +/*
> > > > + * Structure definitions for HVM state that is held by Xen and
> > > > must
> > > > + * be saved along with the domain's memory and device-model
> > > > state.
> > > > + */
> > > > +
> > > > +#ifndef __XEN_PUBLIC_HVM_SAVE_RISCV_H__
> > > > +#define __XEN_PUBLIC_HVM_SAVE_RISCV_H__
> > > > +
> > > > +#endif
> > > > +
> > > > +/*
> > > > + * Local variables:
> > > > + * mode: C
> > > > + * c-file-style: "BSD"
> > > > + * c-basic-offset: 4
> > > > + * tab-width: 4
> > > > + * indent-tabs-mode: nil
> > > > + * End:
> > > > + */
> > > 
> > > Seeing that Arm's is as empty, I wonder why we have it. Julien,
> > > Stefano?
> 
> I'm still curious about the reason here, but ...
> 
> > It seems to exist to satisfy the 'install-tools-public-headers'
> > target:
> > install: cannot stat 'xen/arch-arm/hvm/*.h': No such file or
> > directory
> > Makefile:58: recipe for target 'install' failed
> > make[1]: *** [install] Error 1
> > make[1]: Leaving directory '/builds/xen-
> > project/people/olkur/xen/tools/include'
> > Makefile:44: recipe for target 'install-tools-public-headers'
> > failed
> > 
> > From tools/include/Makefile:
> > install: all
> > ...
> > $(DESTDIR)$(includedir)/xen/arch-arm
> > 	$(INSTALL_DATA) xen/arch-arm/hvm/*.h
> > $(DESTDIR)$(includedir)/xen/arch-arm/hvm
> > ...
> > 
> > We have the following options:
> > 1. Remove the line with $(INSTALL_DATA) xen/arch-arm/hvm/*.h (only
> > save.h is now in this folder, which is empty).
> 
> ... we can't easily remove any existing public header. We can only
> try to
> avoid making the same mistake (even if it's just a minor one) again.
> 
> > 2. Don't touch the Arm part, but for PPC and RISC-V, do the
> > following:
> > #if defined(__i386__) || defined(__x86_64__)
> > #include "../arch-x86/hvm/save.h"
> > #elif defined(__arm__) || defined(__aarch64__)
> > #include "../arch-arm/hvm/save.h"
> > +#elif defined(__powerpc64__) || defined(__riscv)
> > +/* no specific header to include */
> > #else
> > #error "unsupported architecture"
> > #endif
> 
> Yes. Still awaiting Shawn's input here as well, though.
Perhaps you missed the email from Shawn:
https://lore.kernel.org/xen-devel/c2f3280e-2208-496b-a0b5-fda1a2076b3a@raptorengineering.com/

> 
> > 3. Provide an asm-generic version of save.h for Arm, PPC, and RISC-
> > V
> > and use it in public/save.h.
> 
> That's not an option imo - what's under public/ needs to be self-
> contained.
> Stuff there isn't supposed to even know of asm-generic/.
In this case, this is not an option.

~ Oleksii
diff mbox series

Patch

diff --git a/xen/include/public/arch-riscv/hvm/save.h b/xen/include/public/arch-riscv/hvm/save.h
new file mode 100644
index 0000000000..1eb037830d
--- /dev/null
+++ b/xen/include/public/arch-riscv/hvm/save.h
@@ -0,0 +1,20 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Structure definitions for HVM state that is held by Xen and must
+ * be saved along with the domain's memory and device-model state.
+ */
+
+#ifndef __XEN_PUBLIC_HVM_SAVE_RISCV_H__
+#define __XEN_PUBLIC_HVM_SAVE_RISCV_H__
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/hvm/save.h b/xen/include/public/hvm/save.h
index 2cf4238daa..80328c3216 100644
--- a/xen/include/public/hvm/save.h
+++ b/xen/include/public/hvm/save.h
@@ -91,6 +91,8 @@  DECLARE_HVM_SAVE_TYPE(END, 0, struct hvm_save_end);
 #include "../arch-arm/hvm/save.h"
 #elif defined(__powerpc64__)
 #include "../arch-ppc.h"
+#elif defined(__riscv)
+#include "../arch-riscv/hvm/save.h"
 #else
 #error "unsupported architecture"
 #endif