Re: [Xen-devel] [PATCH 02/14 v4] xen/arm: vpl011: Define generic vreg_reg* access functions in vreg.h

2017-06-19 Thread Andre Przywara 
Hi,

On 19/06/17 17:53, Bhupinder Thakur wrote:
> Hi Andre,
> 
> On 19 June 2017 at 15:03, Andre Przywara  wrote:
>> Hi Bhupinder,
>>
>> I think the commit message is a bit misleading.
>> Actually you *rename* functions and their call sites, and also this
>> touches the VGIC code, so shouldn't it mention both in the first line of
>> the commit message? After all this patch really has not much to do with
>> vpl011.
>>
> I will modify the commit message to indicate this commit renames
> vgic_reg* to vreg_reg*
> and modifies all the places where this call is made.

Thanks!

>> On 06/06/17 18:25, Bhupinder Thakur wrote:
>>> This patch redefines the vgic_reg* access functions to vreg_reg* functions.
>>> These are generic functions, which will be used by the vgic emulation code
>>> to access the vgic registers.
>>>
>>> PL011 emulation code will also use vreg_reg* access functions.
>>
>> Also I am sorry to be the bearer of bad news (and also for being the
>> origin of this), but I am afraid you have to rework this when you rebase
>> it against origin/staging, since the ITS emulation has been merged.
>> So while actual conflicts seem to be trivial, there are now many new
>> users of vgic_reg??_* which you have to change as well.
>> Should be rather mechanical, though.
>>
> I will rebase it on origin/staging and merge the changes.

Thanks - and sorry for the mess ;-)

> How do I
> enable ITS code compilation to verify that it is compiling fine with
> new vreg_reg* functions? Is ITS code not compiled in by default?

You need to add "XEN_CONFIG_EXPERT=y" to every make invocation, so both
for any configuration and for the actual build.
If it doesn't prompt you for the ITS, please type:
$ make -C xen menuconfig XEN_CONFIG_EXPERT=y
then select "GICv3 ITS MSI controller support" under "Architecture
Features" (should only be needed once).
You should check xen/.config for having a line with "CONFIG_HAS_ITS=y".

Cheers,
Andre.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/14 v4] xen/arm: vpl011: Define generic vreg_reg* access functions in vreg.h

2017-06-19 Thread Bhupinder Thakur 
Hi Andre,

On 19 June 2017 at 15:03, Andre Przywara  wrote:
> Hi Bhupinder,
>
> I think the commit message is a bit misleading.
> Actually you *rename* functions and their call sites, and also this
> touches the VGIC code, so shouldn't it mention both in the first line of
> the commit message? After all this patch really has not much to do with
> vpl011.
>
I will modify the commit message to indicate this commit renames
vgic_reg* to vreg_reg*
and modifies all the places where this call is made.

> On 06/06/17 18:25, Bhupinder Thakur wrote:
>> This patch redefines the vgic_reg* access functions to vreg_reg* functions.
>> These are generic functions, which will be used by the vgic emulation code
>> to access the vgic registers.
>>
>> PL011 emulation code will also use vreg_reg* access functions.
>
> Also I am sorry to be the bearer of bad news (and also for being the
> origin of this), but I am afraid you have to rework this when you rebase
> it against origin/staging, since the ITS emulation has been merged.
> So while actual conflicts seem to be trivial, there are now many new
> users of vgic_reg??_* which you have to change as well.
> Should be rather mechanical, though.
>
I will rebase it on origin/staging and merge the changes. How do I
enable ITS code compilation to verify that it is compiling fine with
new
vreg_reg* functions? Is ITS code not compiled in by default?

Regards,
Bhupinder

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/14 v4] xen/arm: vpl011: Define generic vreg_reg* access functions in vreg.h

2017-06-19 Thread Andre Przywara 
Hi Bhupinder,

I think the commit message is a bit misleading.
Actually you *rename* functions and their call sites, and also this
touches the VGIC code, so shouldn't it mention both in the first line of
the commit message? After all this patch really has not much to do with
vpl011.

On 06/06/17 18:25, Bhupinder Thakur wrote:
> This patch redefines the vgic_reg* access functions to vreg_reg* functions.
> These are generic functions, which will be used by the vgic emulation code
> to access the vgic registers.
> 
> PL011 emulation code will also use vreg_reg* access functions.

Also I am sorry to be the bearer of bad news (and also for being the
origin of this), but I am afraid you have to rework this when you rebase
it against origin/staging, since the ITS emulation has been merged.
So while actual conflicts seem to be trivial, there are now many new
users of vgic_reg??_* which you have to change as well.
Should be rather mechanical, though.

Cheers,
Andre.

> Signed-off-by: Bhupinder Thakur 
> ---
> CC: ss
> CC: jg
> 
> Changes since v3:
> - Renamed DEFINE_VREG_REG_HELPERS to VREG_REG_HELPERS.
> 
>  xen/arch/arm/vgic-v2.c |  28 +--
>  xen/arch/arm/vgic-v3.c |  40 
>  xen/include/asm-arm/vreg.h | 115 
> ++---
>  3 files changed, 91 insertions(+), 92 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index dc9f95b..3e35a90 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -179,7 +179,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  case VREG32(GICD_CTLR):
>  if ( dabt.size != DABT_WORD ) goto bad_width;
>  vgic_lock(v);
> -*r = vgic_reg32_extract(v->domain->arch.vgic.ctlr, info);
> +*r = vreg_reg32_extract(v->domain->arch.vgic.ctlr, info);
>  vgic_unlock(v);
>  return 1;
>  
> @@ -194,7 +194,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  | DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32);
>  vgic_unlock(v);
>  
> -*r = vgic_reg32_extract(typer, info);
> +*r = vreg_reg32_extract(typer, info);
>  
>  return 1;
>  }
> @@ -205,7 +205,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>   * XXX Do we need a JEP106 manufacturer ID?
>   * Just use the physical h/w value for now
>   */
> -*r = vgic_reg32_extract(0x043b, info);
> +*r = vreg_reg32_extract(0x043b, info);
>  return 1;
>  
>  case VRANGE32(0x00C, 0x01C):
> @@ -226,7 +226,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER, DABT_WORD);
>  if ( rank == NULL) goto read_as_zero;
>  vgic_lock_rank(v, rank, flags);
> -*r = vgic_reg32_extract(rank->ienable, info);
> +*r = vreg_reg32_extract(rank->ienable, info);
>  vgic_unlock_rank(v, rank, flags);
>  return 1;
>  
> @@ -235,7 +235,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER, DABT_WORD);
>  if ( rank == NULL) goto read_as_zero;
>  vgic_lock_rank(v, rank, flags);
> -*r = vgic_reg32_extract(rank->ienable, info);
> +*r = vreg_reg32_extract(rank->ienable, info);
>  vgic_unlock_rank(v, rank, flags);
>  return 1;
>  
> @@ -262,7 +262,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>   gicd_reg - 
> GICD_IPRIORITYR,
>   DABT_WORD)];
>  vgic_unlock_rank(v, rank, flags);
> -*r = vgic_reg32_extract(ipriorityr, info);
> +*r = vreg_reg32_extract(ipriorityr, info);
>  
>  return 1;
>  }
> @@ -280,7 +280,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  vgic_lock_rank(v, rank, flags);
>  itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
>  vgic_unlock_rank(v, rank, flags);
> -*r = vgic_reg32_extract(itargetsr, info);
> +*r = vreg_reg32_extract(itargetsr, info);
>  
>  return 1;
>  }
> @@ -299,7 +299,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>  icfgr = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, 
> DABT_WORD)];
>  vgic_unlock_rank(v, rank, flags);
>  
> -*r = vgic_reg32_extract(icfgr, info);
> +*r = vreg_reg32_extract(icfgr, info);
>  
>  return 1;
>  }
> @@ -424,7 +424,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>  if ( dabt.size != DABT_WORD ) goto bad_width;
>  /* Ignore all but the enable bit */
>  vgic_lock(v);
> -

Re: [Xen-devel] [PATCH 02/14 v4] xen/arm: vpl011: Define generic vreg_reg* access functions in vreg.h

2017-06-09 Thread Julien Grall 

Hi Bhupinder,

On 06/06/17 18:25, Bhupinder Thakur wrote:

-/* N-bit register helpers */
-#define VGIC_REG_HELPERS(sz, offmask)   \
-static inline register_t vgic_reg##sz##_extract(uint##sz##_t reg,   \
-const mmio_info_t *info)\


[...]


+#define VREG_REG_HELPERS(sz, offmask)  
 \
+/* N-bit register helpers */   
 \
+static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg,  
 \
+  const mmio_info_t *info) 
 \


Please don't modify the indentation. There is no reason to do that and 
make the review more difficult to do. This patch should *only* replace 
vgic by vreg.



@@ -211,10 +211,9 @@ static inline void vgic_reg##sz##_clearbits(uint##sz##_t 
*reg,  \
  * unsigned long rather than uint64_t
  */
 #if BITS_PER_LONG == 64
-VGIC_REG_HELPERS(64, 0x7);
+VREG_REG_HELPERS(64, 0x7);
 #endif
-VGIC_REG_HELPERS(32, 0x3);
-


Why did you drop this line?


-#undef VGIC_REG_HELPERS
+VREG_REG_HELPERS(32, 0x3);
+#undef VREG_REG_HELPERS

 #endif /* __ASM_ARM_VREG__ */



Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel