Re: [Minios-devel] [UNIKRAFT PATCHv4 17/43] plat/include: Define macros for Arm64 to access registers

[Julien Grall <julien.grall@xxxxxxx>]

On 09/07/18 10:37, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2018年7月8日 6:13
> > To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> > simon.kuenzer@xxxxxxxxx
> > Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 17/43] plat/include: Define
> > macros for Arm64 to access registers
> >
> > Hi,
> >
> > On 07/06/2018 10:03 AM, Wei Chen wrote:
> > > In the progress of Arm64 system initialization, we need to access
> > > the system registers to configure some CPU features, we also need
> > > to access device registers to make device work. So in this patch,
> > > we define macros to access device registers and system registers.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > ---
> > >    plat/common/include/arm/arm64/cpu.h | 72 +++++++++++++++++++++++++++++
> > >    plat/common/include/arm/cpu.h       | 46 ++++++++++++++++++
> > >    plat/common/include/cpu.h           |  4 +-
> > >    3 files changed, 121 insertions(+), 1 deletion(-)
> > >    create mode 100644 plat/common/include/arm/arm64/cpu.h
> > >    create mode 100644 plat/common/include/arm/cpu.h
> > >
> > > diff --git a/plat/common/include/arm/arm64/cpu.h
> > b/plat/common/include/arm/arm64/cpu.h
> > > new file mode 100644
> > > index 0000000..7c79462
> > > --- /dev/null
> > > +++ b/plat/common/include/arm/arm64/cpu.h
> > > @@ -0,0 +1,72 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause */
> > > +/*
> > > + * Authors: Wei Chen <wei.chen@xxxxxxx>
> > > + *
> > > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> > > + *
> > > + * Redistribution and use in source and binary forms, with or without
> > > + * modification, are permitted provided that the following conditions
> > > + * are met:
> > > + *
> > > + * 1. Redistributions of source code must retain the above copyright
> > > + *    notice, this list of conditions and the following disclaimer.
> > > + * 2. Redistributions in binary form must reproduce the above copyright
> > > + *    notice, this list of conditions and the following disclaimer in the
> > > + *    documentation and/or other materials provided with the distribution.
> > > + * 3. Neither the name of the copyright holder nor the names of its
> > > + *    contributors may be used to endorse or promote products derived from
> > > + *    this software without specific prior written permission.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> > IS"
> > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > THE
> > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > PURPOSE
> > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> > BE
> > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> > THE
> > > + * POSSIBILITY OF SUCH DAMAGE.
> > > + *
> > > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > > + */
> > > +
> > > +#include <inttypes.h>
> > > +
> > > +#define REG_READ8(addr)            \
> > The naming is slightly confusing, you don't really now which register
> > you are reading. How about renaming them to ioread. This would make
> > clearer that you do io access.
> That's fine with me. About this name, I have been commented in
> different projects with different contents : )
> Some guys said, you don't have IO instructions, why you use io as
> Prefix ; )
IO are not only accessed via instructions (e.g outb/inb), they can also 
be memory mapped as it is on Arm.
With the wording "reg", it is not clear whether you are accessing the 
system register or mmio register.
But then you implement outb/inb below that does not make sense for Arm ;).

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel