Re: [PATCH v2 3/9] x86/svm: move declarations used only by svm code from svm.h to private header

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/23/23 13:16, Andrew Cooper wrote:
> On 22/02/2023 12:00 pm, Xenia Ragiadakou wrote:
> > Create a new private header in arch/x86/hvm/svm called svm.h and move there
> > all definitions and declarations that are used solely by svm code.
> >
> > The function svm_invlpga() stays in arch/x86/hvm/svm/svm.h because it is used
> > by arch/x86/hvm/svm/asid.h.
> I'm reasonably sure that all headers in arch/x86/hvm/svm/ other than
> svm.h can move to being private easily.
>
> > Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> > ---
> >
> > Changes in v2:
> >   - new patch, the creation of a private header was suggested by Andrew and Jan
> >
> > I have not added #ifndef guards as it is a private and it should not be
> > included by other headers. However, this is considered a MISRA-C violation
> > ... I 'm not sure what to do.
> Always have guards.  Firstly because that is the decision taken by the
> MISRA working group.
>
> But more importantly, because life is too short to deal with the
> shooting yourself in the foot which will occur from trying to take
> shortcuts like these.
>
>
> > diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> > new file mode 100644
> > index 0000000000..b2ec293078
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/svm/svm.h
> > @@ -0,0 +1,40 @@
> > +#include <xen/types.h>
> Elsewhere, we're retrofitting SPDX tags to all source files, but we're
> already putting tags in new source files.
>
> This one needs to be /* SPDX-License-Identifier: GPL-2.0 */ I think.
>
> > +
> > +static inline void svm_vmload_pa(paddr_t vmcb)
> > +{
> > +    asm volatile (
> > +        ".byte 0x0f,0x01,0xda" /* vmload */
> > +        : : "a" (vmcb) : "memory" );
> > +}
> > +
> > +static inline void svm_vmsave_pa(paddr_t vmcb)
> > +{
> > +    asm volatile (
> > +        ".byte 0x0f,0x01,0xdb" /* vmsave */
> > +        : : "a" (vmcb) : "memory" );
> > +}
> > +
> > +struct cpu_user_regs;
> Looking at this, you could fold patch 1 into this patch and reduce the
> net churn.  It would be fine to say "delete used forward declaration" in
> the commit message, seeing as you're deleting that block of code anyway
> from svm.h
>
> If you want to do this, then I'll skip committing patch 1.
Yes I will do it.

> ~Andrew
--
Xenia