Re: [Xen-devel] [PATCH v12 for-xen-4.5 01/20] common/symbols: Export hypervisor symbols to privileged guest

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/29/2014 09:47 AM, Jan Beulich wrote:
> > > > On 29.09.14 at 15:29, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > On 09/29/2014 02:43 AM, Jan Beulich wrote:
> > > > > > On 26.09.14 at 18:49, <konrad.wilk@xxxxxxxxxx> wrote:
> > > > On Fri, Sep 26, 2014 at 04:10:09PM +0100, Jan Beulich wrote:
> > > > > > > > On 26.09.14 at 16:58, <konrad.wilk@xxxxxxxxxx> wrote:
> > > > > > If I move them just a bit:
> > > > > >
> > > > > >
> > > > > > diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> > > > > > index 4f21b17..b97e476 100644
> > > > > > --- a/xen/include/public/platform.h
> > > > > > +++ b/xen/include/public/platform.h
> > > > > > @@ -538,9 +538,9 @@ struct xenpf_symdata {
> > > > > >                          /*      we reached the end                        */
> > > > > >    
> > > > > >        /* OUT variables */
> > > > > > -    char type;
> > > > > > -    XEN_GUEST_HANDLE(char) name;
> > > > > >        uint64_t address;
> > > > > > +    XEN_GUEST_HANDLE(char) name;
> > > > > > +    char type;
> > > > > >    };
> > > > > >    typedef struct xenpf_symdata xenpf_symdata_t;
> > > > > >    DEFINE_XEN_GUEST_HANDLE(xenpf_symdata_t);
> > > > > >
> > > > > >
> > > > > > 'pahole' is satisfied:
> > > > > >
> > > > > > struct xenpf_symdata {
> > > >    
> > > > > >       uint32_t                   namelen;              /*     0     4 */
> > > >    
> > > > > >       uint32_t                   symnum;               /*     4     4 */
> > > >    
> > > > > >       uint64_t                   address;              /*     8     8 */
> > > >    
> > > > > >       __guest_handle_char        name;                 /*    16     8 */
> > > >    
> > > > > >       char                       type;                 /*    24     1 */
> > > >    
> > > > > >                                                                               
> > > >    
> > > > > >       /* size: 32, cachelines: 1, members: 5 */
> > > >    
> > > > > >       /* padding: 7 */
> > > >    
> > > > > >       /* last cacheline: 32 bytes */
> > > >    
> > > > > > };
> > > > > >
> > > > > >
> > > > > > With that change, Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > > > This change buys us exactly nothing: Structure size doesn't change,
> > > > > and 7 bytes of padding are still there.
> > > > It does allow us to put more parameters (if we want to) at the end of the
> > > > structure instead of fitting them in between.
> > > Regardless of where the gap is, adding further fields in the future
> > > would work only if the code now checked that this field is zero
> > > (which first of all would require it being given a name). I keep
> > > pointing out that this should be done for all padding fields, but I'm
> > > afraid I may have missed doing so on this occasion.
> > I am not sure I understand how setting fields to zero would help with
> > figuring out whether a new fields has been added. I can see how it can
> > in some cases but not in general.
> If you check that padding fields are zero now, meaning can be
> assigned to them later on, while if you allow them to be uninitialized,
> that's not an option.
What if the new field is meant to be zero? You can't guarantee that if 
pad is zero it is still a pad on the "other side" of the call, can you?
Besides, in this particular case, the structure is set up in the caller, 
so presumably it should be the one clearing all pads (which it does, but 
not for this specific reason).
(Also, if we were to add a new field to xenpf_symdata there is plenty of 
space to add a new one (or three) at the end --- it is a part of a 
128-byte union and xenpf_symdata is currently 32 bytes.)
-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel