[PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Currently, the structure vcpu_guest_core_regs is part of the public API.
This implies that any change in the structure should be backward
compatible.

However, the structure is only needed by the tools and Xen. It is also
not expected to be ever used outside of that context. So we could save us
some headache by only declaring the structure for Xen and tools.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This is a follow-up of the discussion [1].

    As this is now Xen and tools only, I am wondering whether the check on
    GNU_C is still necessary. I am happy to send a follow-up patch (or fold
    in this one) if it can be removed.

    [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
---
 xen/include/public/arch-arm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index eb424e8286..e9a86d8eb8 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -197,6 +197,7 @@
     } while ( 0 )
 #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
 
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
 #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
 /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
 # define __DECL_REG(n64, n32) union {          \
@@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
 
 #undef __DECL_REG
 
+#endif
+
 typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn PRIx64
 #define PRIu_xen_pfn PRIu64
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Currently, the structure vcpu_guest_core_regs is part of the public API.
This implies that any change in the structure should be backward
compatible.

However, the structure is only needed by the tools and Xen. It is also
not expected to be ever used outside of that context. So we could save us
some headache by only declaring the structure for Xen and tools.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This is a follow-up of the discussion [1].

    As this is now Xen and tools only, I am wondering whether the check on
    GNU_C is still necessary. I am happy to send a follow-up patch (or fold
    in this one) if it can be removed.

    [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
---
 xen/include/public/arch-arm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index eb424e8286..e9a86d8eb8 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -197,6 +197,7 @@
     } while ( 0 )
 #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
 
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
 #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
 /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
 # define __DECL_REG(n64, n32) union {          \
@@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
 
 #undef __DECL_REG
 
+#endif
+
 typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn PRIx64
 #define PRIu_xen_pfn PRIu64
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Jan Beulich]

>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>     As this is now Xen and tools only, I am wondering whether the check on
>     GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>     in this one) if it can be removed.

I think this should be dropped if it can be without breaking any
part of the build.

> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -197,6 +197,7 @@
>      } while ( 0 )
>  #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>  
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>  #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>  /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>  # define __DECL_REG(n64, n32) union {          \
> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>  
>  #undef __DECL_REG
>  
> +#endif

If I was the maintainer of this code, I'd ask for the struct declaration
to be moved (into the existing #if) rather than adding a 2nd #if.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Jan Beulich]

>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>     As this is now Xen and tools only, I am wondering whether the check on
>     GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>     in this one) if it can be removed.

I think this should be dropped if it can be without breaking any
part of the build.

> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -197,6 +197,7 @@
>      } while ( 0 )
>  #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>  
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>  #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>  /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>  # define __DECL_REG(n64, n32) union {          \
> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>  
>  #undef __DECL_REG
>  
> +#endif

If I was the maintainer of this code, I'd ask for the struct declaration
to be moved (into the existing #if) rather than adding a 2nd #if.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Hi Jan,

On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>      As this is now Xen and tools only, I am wondering whether the check on
>>      GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>      in this one) if it can be removed.
> 
> I think this should be dropped if it can be without breaking any
> part of the build.
This is because all the tools are part of xen.git, right?

>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -197,6 +197,7 @@
>>       } while ( 0 )
>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>   
>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>   #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>   /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>>   # define __DECL_REG(n64, n32) union {          \
>> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>>   
>>   #undef __DECL_REG
>>   
>> +#endif
> 
> If I was the maintainer of this code, I'd ask for the struct declaration
> to be moved (into the existing #if) rather than adding a 2nd #if.

s/2nd/3rd/ ;)

The reason I haven't done that is git will generate a completely 
unrelated diff. So it makes quite difficult to understand the purpose of 
the patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Hi Jan,

On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>      As this is now Xen and tools only, I am wondering whether the check on
>>      GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>      in this one) if it can be removed.
> 
> I think this should be dropped if it can be without breaking any
> part of the build.
This is because all the tools are part of xen.git, right?

>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -197,6 +197,7 @@
>>       } while ( 0 )
>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>   
>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>   #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>   /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>>   # define __DECL_REG(n64, n32) union {          \
>> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>>   
>>   #undef __DECL_REG
>>   
>> +#endif
> 
> If I was the maintainer of this code, I'd ask for the struct declaration
> to be moved (into the existing #if) rather than adding a 2nd #if.

s/2nd/3rd/ ;)

The reason I haven't done that is git will generate a completely 
unrelated diff. So it makes quite difficult to understand the purpose of 
the patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Jan Beulich]

>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>      As this is now Xen and tools only, I am wondering whether the check on
>>>      GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>      in this one) if it can be removed.
>> 
>> I think this should be dropped if it can be without breaking any
>> part of the build.
> This is because all the tools are part of xen.git, right?

Right - no-one else is supposed to define __XEN_TOOLS__, or
if anyone does, they're on their own.

>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -197,6 +197,7 @@
>>>       } while ( 0 )
>>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>>   
>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>>   #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>>   /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>>>   # define __DECL_REG(n64, n32) union {          \
>>> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>>>   
>>>   #undef __DECL_REG
>>>   
>>> +#endif
>> 
>> If I was the maintainer of this code, I'd ask for the struct declaration
>> to be moved (into the existing #if) rather than adding a 2nd #if.
> 
> s/2nd/3rd/ ;)
> 
> The reason I haven't done that is git will generate a completely 
> unrelated diff. So it makes quite difficult to understand the purpose of 
> the patch.

Well, as said - you're the maintainer. I wouldn't be bothered overly
much by a strange diff that might result.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Jan Beulich]

>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>      As this is now Xen and tools only, I am wondering whether the check on
>>>      GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>      in this one) if it can be removed.
>> 
>> I think this should be dropped if it can be without breaking any
>> part of the build.
> This is because all the tools are part of xen.git, right?

Right - no-one else is supposed to define __XEN_TOOLS__, or
if anyone does, they're on their own.

>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -197,6 +197,7 @@
>>>       } while ( 0 )
>>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>>   
>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>>   #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>>   /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>>>   # define __DECL_REG(n64, n32) union {          \
>>> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>>>   
>>>   #undef __DECL_REG
>>>   
>>> +#endif
>> 
>> If I was the maintainer of this code, I'd ask for the struct declaration
>> to be moved (into the existing #if) rather than adding a 2nd #if.
> 
> s/2nd/3rd/ ;)
> 
> The reason I haven't done that is git will generate a completely 
> unrelated diff. So it makes quite difficult to understand the purpose of 
> the patch.

Well, as said - you're the maintainer. I wouldn't be bothered overly
much by a strange diff that might result.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Hi Jan,

On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>       As this is now Xen and tools only, I am wondering whether the check on
>>>>       GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>       in this one) if it can be removed.
>>>
>>> I think this should be dropped if it can be without breaking any
>>> part of the build.
>> This is because all the tools are part of xen.git, right?
> 
> Right - no-one else is supposed to define __XEN_TOOLS__, or
> if anyone does, they're on their own.

Thanks for the information. I will do a full build check.

> 
>>>> --- a/xen/include/public/arch-arm.h
>>>> +++ b/xen/include/public/arch-arm.h
>>>> @@ -197,6 +197,7 @@
>>>>        } while ( 0 )
>>>>    #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>>>    
>>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>>>    #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>>>    /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>>>>    # define __DECL_REG(n64, n32) union {          \
>>>> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>>>>    
>>>>    #undef __DECL_REG
>>>>    
>>>> +#endif
>>>
>>> If I was the maintainer of this code, I'd ask for the struct declaration
>>> to be moved (into the existing #if) rather than adding a 2nd #if.
>>
>> s/2nd/3rd/ ;)
>>
>> The reason I haven't done that is git will generate a completely
>> unrelated diff. So it makes quite difficult to understand the purpose of
>> the patch.
> 
> Well, as said - you're the maintainer. I wouldn't be bothered overly
> much by a strange diff that might result.

I will wait on Stefano's input.


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Hi Jan,

On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>       As this is now Xen and tools only, I am wondering whether the check on
>>>>       GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>       in this one) if it can be removed.
>>>
>>> I think this should be dropped if it can be without breaking any
>>> part of the build.
>> This is because all the tools are part of xen.git, right?
> 
> Right - no-one else is supposed to define __XEN_TOOLS__, or
> if anyone does, they're on their own.

Thanks for the information. I will do a full build check.

> 
>>>> --- a/xen/include/public/arch-arm.h
>>>> +++ b/xen/include/public/arch-arm.h
>>>> @@ -197,6 +197,7 @@
>>>>        } while ( 0 )
>>>>    #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>>>    
>>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>>>>    #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>>>    /* Anonymous union includes both 32- and 64-bit names (e.g., r0/x0). */
>>>>    # define __DECL_REG(n64, n32) union {          \
>>>> @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
>>>>    
>>>>    #undef __DECL_REG
>>>>    
>>>> +#endif
>>>
>>> If I was the maintainer of this code, I'd ask for the struct declaration
>>> to be moved (into the existing #if) rather than adding a 2nd #if.
>>
>> s/2nd/3rd/ ;)
>>
>> The reason I haven't done that is git will generate a completely
>> unrelated diff. So it makes quite difficult to understand the purpose of
>> the patch.
> 
> Well, as said - you're the maintainer. I wouldn't be bothered overly
> much by a strange diff that might result.

I will wait on Stefano's input.


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Stefano Stabellini]

On Tue, 21 May 2019, Julien Grall wrote:
> Hi Jan,
> 
> On 5/21/19 10:43 AM, Jan Beulich wrote:
> > > > > On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
> > > On 5/21/19 10:26 AM, Jan Beulich wrote:
> > > > > > > On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
> > > > >       As this is now Xen and tools only, I am wondering whether the
> > > > > check on
> > > > >       GNU_C is still necessary. I am happy to send a follow-up patch
> > > > > (or fold
> > > > >       in this one) if it can be removed.
> > > > 
> > > > I think this should be dropped if it can be without breaking any
> > > > part of the build.
> > > This is because all the tools are part of xen.git, right?
> > 
> > Right - no-one else is supposed to define __XEN_TOOLS__, or
> > if anyone does, they're on their own.
> 
> Thanks for the information. I will do a full build check.
> 
> > 
> > > > > --- a/xen/include/public/arch-arm.h
> > > > > +++ b/xen/include/public/arch-arm.h
> > > > > @@ -197,6 +197,7 @@
> > > > >        } while ( 0 )
> > > > >    #define set_xen_guest_handle(hnd, val)
> > > > > set_xen_guest_handle_raw(hnd, val)
> > > > >    +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> > > > >    #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> > > > >    /* Anonymous union includes both 32- and 64-bit names (e.g.,
> > > > > r0/x0). */
> > > > >    # define __DECL_REG(n64, n32) union {          \
> > > > > @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
> > > > >       #undef __DECL_REG
> > > > >    +#endif
> > > > 
> > > > If I was the maintainer of this code, I'd ask for the struct declaration
> > > > to be moved (into the existing #if) rather than adding a 2nd #if.
> > > 
> > > s/2nd/3rd/ ;)
> > > 
> > > The reason I haven't done that is git will generate a completely
> > > unrelated diff. So it makes quite difficult to understand the purpose of
> > > the patch.
> > 
> > Well, as said - you're the maintainer. I wouldn't be bothered overly
> > much by a strange diff that might result.
> 
> I will wait on Stefano's input.

Yes, please follow Jan's advice, thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Stefano Stabellini]

On Tue, 21 May 2019, Julien Grall wrote:
> Hi Jan,
> 
> On 5/21/19 10:43 AM, Jan Beulich wrote:
> > > > > On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
> > > On 5/21/19 10:26 AM, Jan Beulich wrote:
> > > > > > > On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
> > > > >       As this is now Xen and tools only, I am wondering whether the
> > > > > check on
> > > > >       GNU_C is still necessary. I am happy to send a follow-up patch
> > > > > (or fold
> > > > >       in this one) if it can be removed.
> > > > 
> > > > I think this should be dropped if it can be without breaking any
> > > > part of the build.
> > > This is because all the tools are part of xen.git, right?
> > 
> > Right - no-one else is supposed to define __XEN_TOOLS__, or
> > if anyone does, they're on their own.
> 
> Thanks for the information. I will do a full build check.
> 
> > 
> > > > > --- a/xen/include/public/arch-arm.h
> > > > > +++ b/xen/include/public/arch-arm.h
> > > > > @@ -197,6 +197,7 @@
> > > > >        } while ( 0 )
> > > > >    #define set_xen_guest_handle(hnd, val)
> > > > > set_xen_guest_handle_raw(hnd, val)
> > > > >    +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> > > > >    #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> > > > >    /* Anonymous union includes both 32- and 64-bit names (e.g.,
> > > > > r0/x0). */
> > > > >    # define __DECL_REG(n64, n32) union {          \
> > > > > @@ -272,6 +273,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
> > > > >       #undef __DECL_REG
> > > > >    +#endif
> > > > 
> > > > If I was the maintainer of this code, I'd ask for the struct declaration
> > > > to be moved (into the existing #if) rather than adding a 2nd #if.
> > > 
> > > s/2nd/3rd/ ;)
> > > 
> > > The reason I haven't done that is git will generate a completely
> > > unrelated diff. So it makes quite difficult to understand the purpose of
> > > the patch.
> > 
> > Well, as said - you're the maintainer. I wouldn't be bothered overly
> > much by a strange diff that might result.
> 
> I will wait on Stefano's input.

Yes, please follow Jan's advice, thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

On 21/05/2019 10:55, Julien Grall wrote:
> Hi Jan,
> 
> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>>       As this is now Xen and tools only, I am wondering whether the check on
>>>>>       GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>>       in this one) if it can be removed.
>>>>
>>>> I think this should be dropped if it can be without breaking any
>>>> part of the build.
>>> This is because all the tools are part of xen.git, right?
>>
>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>> if anyone does, they're on their own.
> 
> Thanks for the information. I will do a full build check.

I thought about this again, long term there are an attempt to build xen with 
other compiler not necessarily supporting GNU C extension.

While this would probably not be the only place that need to be reworked, we 
would have to revert part of this change. So I will not drop the #ifdef here.

I will resend the patch next week to give some time for more feedback.


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

On 21/05/2019 10:55, Julien Grall wrote:
> Hi Jan,
> 
> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>>       As this is now Xen and tools only, I am wondering whether the check on
>>>>>       GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>>       in this one) if it can be removed.
>>>>
>>>> I think this should be dropped if it can be without breaking any
>>>> part of the build.
>>> This is because all the tools are part of xen.git, right?
>>
>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>> if anyone does, they're on their own.
> 
> Thanks for the information. I will do a full build check.

I thought about this again, long term there are an attempt to build xen with 
other compiler not necessarily supporting GNU C extension.

While this would probably not be the only place that need to be reworked, we 
would have to revert part of this change. So I will not drop the #ifdef here.

I will resend the patch next week to give some time for more feedback.


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Jan Beulich]

>>> On 22.05.19 at 14:20, <julien.grall@arm.com> wrote:

> 
> On 21/05/2019 10:55, Julien Grall wrote:
>> Hi Jan,
>> 
>> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>>>       As this is now Xen and tools only, I am wondering whether the check on
>>>>>>       GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>>>       in this one) if it can be removed.
>>>>>
>>>>> I think this should be dropped if it can be without breaking any
>>>>> part of the build.
>>>> This is because all the tools are part of xen.git, right?
>>>
>>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>>> if anyone does, they're on their own.
>> 
>> Thanks for the information. I will do a full build check.
> 
> I thought about this again, long term there are an attempt to build xen with 
> other compiler not necessarily supporting GNU C extension.
> While this would probably not be the only place that need to be reworked, we 
> would have to revert part of this change. So I will not drop the #ifdef here.

Well, I don't know how it is for Arm, but on x86 we actually use the
"extended" naming quite extensively, so building with a compiler
that doesn't support this extension is not really an option there.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Jan Beulich]

>>> On 22.05.19 at 14:20, <julien.grall@arm.com> wrote:

> 
> On 21/05/2019 10:55, Julien Grall wrote:
>> Hi Jan,
>> 
>> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>>>       As this is now Xen and tools only, I am wondering whether the check on
>>>>>>       GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>>>       in this one) if it can be removed.
>>>>>
>>>>> I think this should be dropped if it can be without breaking any
>>>>> part of the build.
>>>> This is because all the tools are part of xen.git, right?
>>>
>>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>>> if anyone does, they're on their own.
>> 
>> Thanks for the information. I will do a full build check.
> 
> I thought about this again, long term there are an attempt to build xen with 
> other compiler not necessarily supporting GNU C extension.
> While this would probably not be the only place that need to be reworked, we 
> would have to revert part of this change. So I will not drop the #ifdef here.

Well, I don't know how it is for Arm, but on x86 we actually use the
"extended" naming quite extensively, so building with a compiler
that doesn't support this extension is not really an option there.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

(+Artem)

Hi Jan,

On 22/05/2019 13:29, Jan Beulich wrote:
>>>> On 22.05.19 at 14:20, <julien.grall@arm.com> wrote:
> 
>>
>> On 21/05/2019 10:55, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>>>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>>>>        As this is now Xen and tools only, I am wondering whether the check on
>>>>>>>        GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>>>>        in this one) if it can be removed.
>>>>>>
>>>>>> I think this should be dropped if it can be without breaking any
>>>>>> part of the build.
>>>>> This is because all the tools are part of xen.git, right?
>>>>
>>>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>>>> if anyone does, they're on their own.
>>>
>>> Thanks for the information. I will do a full build check.
>>
>> I thought about this again, long term there are an attempt to build xen with
>> other compiler not necessarily supporting GNU C extension.
>> While this would probably not be the only place that need to be reworked, we
>> would have to revert part of this change. So I will not drop the #ifdef here.
> 
> Well, I don't know how it is for Arm, but on x86 we actually use the
> "extended" naming quite extensively, so building with a compiler
> that doesn't support this extension is not really an option there.

For the Arm, I think only cpu_user_regs is using "extended" naming. It should be 
possible to remove it without too much trouble here.

@Artem, is there any restriction to use anonymous union in functional safety?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

(+Artem)

Hi Jan,

On 22/05/2019 13:29, Jan Beulich wrote:
>>>> On 22.05.19 at 14:20, <julien.grall@arm.com> wrote:
> 
>>
>> On 21/05/2019 10:55, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>>>> On 21.05.19 at 11:35, <julien.grall@arm.com> wrote:
>>>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>>>> On 20.05.19 at 20:12, <julien.grall@arm.com> wrote:
>>>>>>>        As this is now Xen and tools only, I am wondering whether the check on
>>>>>>>        GNU_C is still necessary. I am happy to send a follow-up patch (or fold
>>>>>>>        in this one) if it can be removed.
>>>>>>
>>>>>> I think this should be dropped if it can be without breaking any
>>>>>> part of the build.
>>>>> This is because all the tools are part of xen.git, right?
>>>>
>>>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>>>> if anyone does, they're on their own.
>>>
>>> Thanks for the information. I will do a full build check.
>>
>> I thought about this again, long term there are an attempt to build xen with
>> other compiler not necessarily supporting GNU C extension.
>> While this would probably not be the only place that need to be reworked, we
>> would have to revert part of this change. So I will not drop the #ifdef here.
> 
> Well, I don't know how it is for Arm, but on x86 we actually use the
> "extended" naming quite extensively, so building with a compiler
> that doesn't support this extension is not really an option there.

For the Arm, I think only cpu_user_regs is using "extended" naming. It should be 
possible to remove it without too much trouble here.

@Artem, is there any restriction to use anonymous union in functional safety?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Artem Mygaiev]

Hello Julien, Jan

On Wed, 2019-05-22 at 14:00 +0100, Julien Grall wrote:
> (+Artem)
> 
> Hi Jan,
> 
> On 22/05/2019 13:29, Jan Beulich wrote:
> > > > > On 22.05.19 at 14:20, <
> > > > > julien.grall@arm.com
> > > > > > wrote:
> > > On 21/05/2019 10:55, Julien Grall wrote:
> > > > Hi Jan,
> > > > 
> > > > On 5/21/19 10:43 AM, Jan Beulich wrote:
> > > > > > > > On 21.05.19 at 11:35, <
> > > > > > > > julien.grall@arm.com
> > > > > > > > > wrote:
> > > > > > 
> > > > > > On 5/21/19 10:26 AM, Jan Beulich wrote:
> > > > > > > > > > On 20.05.19 at 20:12, <
> > > > > > > > > > julien.grall@arm.com
> > > > > > > > > > > wrote:
> > > > > > > > 
> > > > > > > >        As this is now Xen and tools only, I am
> > > > > > > > wondering whether the check on
> > > > > > > >        GNU_C is still necessary. I am happy to send a
> > > > > > > > follow-up patch (or fold
> > > > > > > >        in this one) if it can be removed.
> > > > > > > 
> > > > > > > I think this should be dropped if it can be without
> > > > > > > breaking any
> > > > > > > part of the build.
> > > > > > 
> > > > > > This is because all the tools are part of xen.git, right?
> > > > > 
> > > > > Right - no-one else is supposed to define __XEN_TOOLS__, or
> > > > > if anyone does, they're on their own.
> > > > 
> > > > Thanks for the information. I will do a full build check.
> > > 
> > > I thought about this again, long term there are an attempt to
> > > build xen with
> > > other compiler not necessarily supporting GNU C extension.
> > > While this would probably not be the only place that need to be
> > > reworked, we
> > > would have to revert part of this change. So I will not drop the
> > > #ifdef here.
> > 
> > Well, I don't know how it is for Arm, but on x86 we actually use
> > the
> > "extended" naming quite extensively, so building with a compiler
> > that doesn't support this extension is not really an option there.
> 
> For the Arm, I think only cpu_user_regs is using "extended" naming.
> It should be 
> possible to remove it without too much trouble here.
> 
> @Artem, is there any restriction to use anonymous union in functional
> safety?
> 

In general, unions are not allowed in safety regulated programming,
they always require a "deviation" - e.g. unions use for data packing is
usually accepted disregarding anonymous or not.

Couple of other things I wanted to mention:
1. all protective programming standards e.g. MISRA recommend reducing
visibility of functions and variables to reduce API surface ans thus
need for test coverage and systematic fault probability.
2. current implementation xen tools are very hard to use in safety for
many reasons, I hope to follow up on this soon...

 -- Artem
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Artem Mygaiev]

Hello Julien, Jan

On Wed, 2019-05-22 at 14:00 +0100, Julien Grall wrote:
> (+Artem)
> 
> Hi Jan,
> 
> On 22/05/2019 13:29, Jan Beulich wrote:
> > > > > On 22.05.19 at 14:20, <
> > > > > julien.grall@arm.com
> > > > > > wrote:
> > > On 21/05/2019 10:55, Julien Grall wrote:
> > > > Hi Jan,
> > > > 
> > > > On 5/21/19 10:43 AM, Jan Beulich wrote:
> > > > > > > > On 21.05.19 at 11:35, <
> > > > > > > > julien.grall@arm.com
> > > > > > > > > wrote:
> > > > > > 
> > > > > > On 5/21/19 10:26 AM, Jan Beulich wrote:
> > > > > > > > > > On 20.05.19 at 20:12, <
> > > > > > > > > > julien.grall@arm.com
> > > > > > > > > > > wrote:
> > > > > > > > 
> > > > > > > >        As this is now Xen and tools only, I am
> > > > > > > > wondering whether the check on
> > > > > > > >        GNU_C is still necessary. I am happy to send a
> > > > > > > > follow-up patch (or fold
> > > > > > > >        in this one) if it can be removed.
> > > > > > > 
> > > > > > > I think this should be dropped if it can be without
> > > > > > > breaking any
> > > > > > > part of the build.
> > > > > > 
> > > > > > This is because all the tools are part of xen.git, right?
> > > > > 
> > > > > Right - no-one else is supposed to define __XEN_TOOLS__, or
> > > > > if anyone does, they're on their own.
> > > > 
> > > > Thanks for the information. I will do a full build check.
> > > 
> > > I thought about this again, long term there are an attempt to
> > > build xen with
> > > other compiler not necessarily supporting GNU C extension.
> > > While this would probably not be the only place that need to be
> > > reworked, we
> > > would have to revert part of this change. So I will not drop the
> > > #ifdef here.
> > 
> > Well, I don't know how it is for Arm, but on x86 we actually use
> > the
> > "extended" naming quite extensively, so building with a compiler
> > that doesn't support this extension is not really an option there.
> 
> For the Arm, I think only cpu_user_regs is using "extended" naming.
> It should be 
> possible to remove it without too much trouble here.
> 
> @Artem, is there any restriction to use anonymous union in functional
> safety?
> 

In general, unions are not allowed in safety regulated programming,
they always require a "deviation" - e.g. unions use for data packing is
usually accepted disregarding anonymous or not.

Couple of other things I wanted to mention:
1. all protective programming standards e.g. MISRA recommend reducing
visibility of functions and variables to reduce API surface ans thus
need for test coverage and systematic fault probability.
2. current implementation xen tools are very hard to use in safety for
many reasons, I hope to follow up on this soon...

 -- Artem
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Hi Artem,

On 5/22/19 7:05 PM, Artem Mygaiev wrote:
> On Wed, 2019-05-22 at 14:00 +0100, Julien Grall wrote:
>> On 22/05/2019 13:29, Jan Beulich wrote:
>>>>>> On 22.05.19 at 14:20, <
>>>>>> julien.grall@arm.com
>>>>>>> wrote:
>>>> On 21/05/2019 10:55, Julien Grall wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>>>>>> On 21.05.19 at 11:35, <
>>>>>>>>> julien.grall@arm.com
>>>>>>>>>> wrote:
>>>>>>>
>>>>>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>>>>>> On 20.05.19 at 20:12, <
>>>>>>>>>>> julien.grall@arm.com
>>>>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>         As this is now Xen and tools only, I am
>>>>>>>>> wondering whether the check on
>>>>>>>>>         GNU_C is still necessary. I am happy to send a
>>>>>>>>> follow-up patch (or fold
>>>>>>>>>         in this one) if it can be removed.
>>>>>>>>
>>>>>>>> I think this should be dropped if it can be without
>>>>>>>> breaking any
>>>>>>>> part of the build
>>>>>>>
>>>>>>> This is because all the tools are part of xen.git, right?
>>>>>>
>>>>>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>>>>>> if anyone does, they're on their own.
>>>>>
>>>>> Thanks for the information. I will do a full build check.
>>>>
>>>> I thought about this again, long term there are an attempt to
>>>> build xen with
>>>> other compiler not necessarily supporting GNU C extension.
>>>> While this would probably not be the only place that need to be
>>>> reworked, we
>>>> would have to revert part of this change. So I will not drop the
>>>> #ifdef here.
>>>
>>> Well, I don't know how it is for Arm, but on x86 we actually use
>>> the
>>> "extended" naming quite extensively, so building with a compiler
>>> that doesn't support this extension is not really an option there.
>>
>> For the Arm, I think only cpu_user_regs is using "extended" naming.
>> It should be
>> possible to remove it without too much trouble here.
>>
>> @Artem, is there any restriction to use anonymous union in functional
>> safety?
>>
> 
> In general, unions are not allowed in safety regulated programming,
> they always require a "deviation" - e.g. unions use for data packing is
> usually accepted disregarding anonymous or not.

That's good to know. I am going to keep for now the two definitions of 
__DECL_REG. We can remove them later on if it is not necessary.

> 
> Couple of other things I wanted to mention:
> 1. all protective programming standards e.g. MISRA recommend reducing
> visibility of functions and variables to reduce API surface ans thus
> need for test coverage and systematic fault probability.

In general, we want to limit the API exposed to guest as this is stable.
Let us know if you see other places where we could potentially reduce 
the API without impacting existing guest.

> 2. current implementation xen tools are very hard to use in safety for
> many reasons, I hope to follow up on this soon...

Thank you for the feedback!

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

[Julien Grall]

Hi Artem,

On 5/22/19 7:05 PM, Artem Mygaiev wrote:
> On Wed, 2019-05-22 at 14:00 +0100, Julien Grall wrote:
>> On 22/05/2019 13:29, Jan Beulich wrote:
>>>>>> On 22.05.19 at 14:20, <
>>>>>> julien.grall@arm.com
>>>>>>> wrote:
>>>> On 21/05/2019 10:55, Julien Grall wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 5/21/19 10:43 AM, Jan Beulich wrote:
>>>>>>>>> On 21.05.19 at 11:35, <
>>>>>>>>> julien.grall@arm.com
>>>>>>>>>> wrote:
>>>>>>>
>>>>>>> On 5/21/19 10:26 AM, Jan Beulich wrote:
>>>>>>>>>>> On 20.05.19 at 20:12, <
>>>>>>>>>>> julien.grall@arm.com
>>>>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>         As this is now Xen and tools only, I am
>>>>>>>>> wondering whether the check on
>>>>>>>>>         GNU_C is still necessary. I am happy to send a
>>>>>>>>> follow-up patch (or fold
>>>>>>>>>         in this one) if it can be removed.
>>>>>>>>
>>>>>>>> I think this should be dropped if it can be without
>>>>>>>> breaking any
>>>>>>>> part of the build
>>>>>>>
>>>>>>> This is because all the tools are part of xen.git, right?
>>>>>>
>>>>>> Right - no-one else is supposed to define __XEN_TOOLS__, or
>>>>>> if anyone does, they're on their own.
>>>>>
>>>>> Thanks for the information. I will do a full build check.
>>>>
>>>> I thought about this again, long term there are an attempt to
>>>> build xen with
>>>> other compiler not necessarily supporting GNU C extension.
>>>> While this would probably not be the only place that need to be
>>>> reworked, we
>>>> would have to revert part of this change. So I will not drop the
>>>> #ifdef here.
>>>
>>> Well, I don't know how it is for Arm, but on x86 we actually use
>>> the
>>> "extended" naming quite extensively, so building with a compiler
>>> that doesn't support this extension is not really an option there.
>>
>> For the Arm, I think only cpu_user_regs is using "extended" naming.
>> It should be
>> possible to remove it without too much trouble here.
>>
>> @Artem, is there any restriction to use anonymous union in functional
>> safety?
>>
> 
> In general, unions are not allowed in safety regulated programming,
> they always require a "deviation" - e.g. unions use for data packing is
> usually accepted disregarding anonymous or not.

That's good to know. I am going to keep for now the two definitions of 
__DECL_REG. We can remove them later on if it is not necessary.

> 
> Couple of other things I wanted to mention:
> 1. all protective programming standards e.g. MISRA recommend reducing
> visibility of functions and variables to reduce API surface ans thus
> need for test coverage and systematic fault probability.

In general, we want to limit the API exposed to guest as this is stable.
Let us know if you see other places where we could potentially reduce 
the API without impacting existing guest.

> 2. current implementation xen tools are very hard to use in safety for
> many reasons, I hope to follow up on this soon...

Thank you for the feedback!

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel