[Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-04-28 Thread Bhupinder Thakur 
Add emulation code to emulate read/write access to pl011 registers
and pl011 interrupts:

- Emulate DR read/write by reading and writing from/to the IN
  and OUT ring buffers and raising an event to the backend when
  there is data in the OUT ring buffer and injecting an interrupt
  to the guest when there is data in the IN ring buffer

- Other registers are related to interrupt management and
  essentially control when interrupts are delivered to the guest

The SBSA compliant pl011 uart is covered in Appendix B of
https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf

Signed-off-by: Bhupinder Thakur 
---

Changes since v1:

- Removed the optimiztion related to sendiing events to xenconsole 
- Use local variables as ring buffer indices while using the ring buffer

 xen/arch/arm/Kconfig |   5 +
 xen/arch/arm/Makefile|   1 +
 xen/arch/arm/vpl011.c| 340 +++
 xen/include/asm-arm/domain.h |   3 +
 xen/include/asm-arm/pl011-uart.h |   2 +
 xen/include/public/arch-arm.h|   8 +
 xen/include/xen/vpl011.h |  74 +
 7 files changed, 433 insertions(+)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/xen/vpl011.h

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index d46b98c..c1a0e7f 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -50,6 +50,11 @@ config HAS_ITS
 prompt "GICv3 ITS MSI controller support" if EXPERT = "y"
 depends on HAS_GICV3
 
+config VPL011_CONSOLE
+   bool "Emulated pl011 console support"
+   default y
+   ---help---
+ Allows a guest to use pl011 UART as a console
 endmenu
 
 menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..15efc13 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -52,6 +52,7 @@ obj-y += vm_event.o
 obj-y += vtimer.o
 obj-y += vpsci.o
 obj-y += vuart.o
+obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o
 
 #obj-bin-y += o
 
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
new file mode 100644
index 000..51abade
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,340 @@
+/*
+ * arch/arm/vpl011.c
+ *
+ * Virtual PL011 UART
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+unsigned int vpl011_reg_mask[] = {0xff, 0x, 0x};
+
+static void vgic_inject_vpl011_spi(struct domain *d)
+{
+struct vpl011_s *vpl011 = &d->arch.vpl011;
+
+if ( (vpl011->uartris & vpl011->uartimsc) )
+vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
+}
+
+static void vpl011_read_data(struct domain *d, uint8_t *data)
+{
+unsigned long flags;
+struct vpl011_s *vpl011 = &d->arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+
+/*
+ * Initialize the data so that even if there is no data in ring buffer
+ * 0 is returned.
+ */
+*data = 0;
+
+VPL011_LOCK(d, flags);
+
+/*
+ * It is expected that there will be data in the ring buffer when this
+ * function is called since the guest is expected to read the data register
+ * only if the TXFE flag is not set.
+ * If the guest still does read when TXFE bit is set then 0 will be 
returned.
+ */
+if ( !VPL011_IN_RING_EMPTY(intf) )
+{
+uint32_t in_cons = intf->in_cons;
+*data = intf->in[MASK_XENCONS_IDX(in_cons, intf->in)];
+smp_mb();
+intf->in_cons = in_cons + 1;
+}
+
+if ( VPL011_IN_RING_EMPTY(intf) )
+{
+vpl011->uartfr |= (RXFE);
+vpl011->uartris &= ~(RXI);
+}
+vpl011->uartfr &= ~(RXFF);
+VPL011_UNLOCK(d, flags);
+
+notify_via_xen_event_channel(d, vpl011->evtchn);
+}
+
+static void vpl011_write_data(struct domain *d, uint8_t data)
+{
+unsigned long flags;
+struct vpl011_s *vpl011 = &d->arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+
+VPL011_LOCK(d, flags);
+
+/*
+ * It is expected that the ring is not full when this function is called
+ * as the guest is expected to write to the data register only when the
+ * TXFF flag is not set.
+ * In case the guest does write even when the TXFF flag is set then the
+ * data will be silently dropped.
+ */
+

Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-04-28 Thread Stefano Stabellini 
On Fri, 28 Apr 2017, Bhupinder Thakur wrote:
> Add emulation code to emulate read/write access to pl011 registers
> and pl011 interrupts:
> 
> - Emulate DR read/write by reading and writing from/to the IN
>   and OUT ring buffers and raising an event to the backend when
>   there is data in the OUT ring buffer and injecting an interrupt
>   to the guest when there is data in the IN ring buffer
> 
> - Other registers are related to interrupt management and
>   essentially control when interrupts are delivered to the guest
> 
> The SBSA compliant pl011 uart is covered in Appendix B of
> https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf
> 
> Signed-off-by: Bhupinder Thakur 
> ---
> 
> Changes since v1:
> 
> - Removed the optimiztion related to sendiing events to xenconsole 
> - Use local variables as ring buffer indices while using the ring buffer
> 
>  xen/arch/arm/Kconfig |   5 +
>  xen/arch/arm/Makefile|   1 +
>  xen/arch/arm/vpl011.c| 340 
> +++
>  xen/include/asm-arm/domain.h |   3 +
>  xen/include/asm-arm/pl011-uart.h |   2 +
>  xen/include/public/arch-arm.h|   8 +
>  xen/include/xen/vpl011.h |  74 +
>  7 files changed, 433 insertions(+)
>  create mode 100644 xen/arch/arm/vpl011.c
>  create mode 100644 xen/include/xen/vpl011.h
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index d46b98c..c1a0e7f 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -50,6 +50,11 @@ config HAS_ITS
>  prompt "GICv3 ITS MSI controller support" if EXPERT = "y"
>  depends on HAS_GICV3
>  
> +config VPL011_CONSOLE
> + bool "Emulated pl011 console support"
> + default y
> + ---help---
> +   Allows a guest to use pl011 UART as a console
>  endmenu
>  
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..15efc13 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -52,6 +52,7 @@ obj-y += vm_event.o
>  obj-y += vtimer.o
>  obj-y += vpsci.o
>  obj-y += vuart.o
> +obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o
>  
>  #obj-bin-y += o
>  
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> new file mode 100644
> index 000..51abade
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
> @@ -0,0 +1,340 @@
> +/*
> + * arch/arm/vpl011.c
> + *
> + * Virtual PL011 UART
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +unsigned int vpl011_reg_mask[] = {0xff, 0x, 0x};
> +
> +static void vgic_inject_vpl011_spi(struct domain *d)
> +{
> +struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> +if ( (vpl011->uartris & vpl011->uartimsc) )
> +vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> +}
> +
> +static void vpl011_read_data(struct domain *d, uint8_t *data)
> +{
> +unsigned long flags;
> +struct vpl011_s *vpl011 = &d->arch.vpl011;
> +struct xencons_interface *intf = vpl011->ring_buf;
> +
> +/*
> + * Initialize the data so that even if there is no data in ring buffer
> + * 0 is returned.
> + */
> +*data = 0;
> +
> +VPL011_LOCK(d, flags);
> +
> +/*
> + * It is expected that there will be data in the ring buffer when this
> + * function is called since the guest is expected to read the data 
> register
> + * only if the TXFE flag is not set.
> + * If the guest still does read when TXFE bit is set then 0 will be 
> returned.
> + */
> +if ( !VPL011_IN_RING_EMPTY(intf) )
> +{
> +uint32_t in_cons = intf->in_cons;

Use XENCONS_RING_IDX for the indexes type everywhere in this file.


> +*data = intf->in[MASK_XENCONS_IDX(in_cons, intf->in)];
> +smp_mb();
> +intf->in_cons = in_cons + 1;
> +}
> +
> +if ( VPL011_IN_RING_EMPTY(intf) )
> +{
> +vpl011->uartfr |= (RXFE);
> +vpl011->uartris &= ~(RXI);
> +}
> +vpl011->uartfr &= ~(RXFF);
> +VPL011_UNLOCK(d, flags);
> +
> +notify_via_xen_event_channel(d, vpl011->evtchn);
> +}
> +
> +static void vpl011_write_data(struct domain *d, uint8_t data)
> +{
> +unsigned long flags;
> +struct vpl011_s *vpl011 = &d->arch.vp

Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-05-02 Thread Jan Beulich 
>>> On 28.04.17 at 18:01,  wrote:
>  xen/arch/arm/Kconfig |   5 +
>  xen/arch/arm/Makefile|   1 +
>  xen/arch/arm/vpl011.c| 340 
> +++
>  xen/include/asm-arm/domain.h |   3 +
>  xen/include/asm-arm/pl011-uart.h |   2 +
>  xen/include/public/arch-arm.h|   8 +
>  xen/include/xen/vpl011.h |  74 +
>  7 files changed, 433 insertions(+)
>  create mode 100644 xen/arch/arm/vpl011.c
>  create mode 100644 xen/include/xen/vpl011.h

Considering the implementation is ARM specific (as is pl011 itself)
I consider the header file misplaced - it ought to go under arch-arm/.

With that (and this also applies to other patches in this series)
please make sure you copy all involved maintainers _for a
particular patch_, but not all involved for some patch somewhere
in the series (i.e. you want to have per-patch Cc lists, not a single
one for the entire series).

Jan


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

Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-05-02 Thread Julien Grall 

Hi Bhupinder,

On 28/04/17 17:01, Bhupinder Thakur wrote:

Add emulation code to emulate read/write access to pl011 registers
and pl011 interrupts:

- Emulate DR read/write by reading and writing from/to the IN
  and OUT ring buffers and raising an event to the backend when
  there is data in the OUT ring buffer and injecting an interrupt
  to the guest when there is data in the IN ring buffer

- Other registers are related to interrupt management and
  essentially control when interrupts are delivered to the guest

The SBSA compliant pl011 uart is covered in Appendix B of
https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf

Signed-off-by: Bhupinder Thakur 
---

Changes since v1:

- Removed the optimiztion related to sendiing events to xenconsole
- Use local variables as ring buffer indices while using the ring buffer

 xen/arch/arm/Kconfig |   5 +
 xen/arch/arm/Makefile|   1 +
 xen/arch/arm/vpl011.c| 340 +++
 xen/include/asm-arm/domain.h |   3 +
 xen/include/asm-arm/pl011-uart.h |   2 +
 xen/include/public/arch-arm.h|   8 +
 xen/include/xen/vpl011.h |  74 +
 7 files changed, 433 insertions(+)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/xen/vpl011.h

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index d46b98c..c1a0e7f 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -50,6 +50,11 @@ config HAS_ITS
 prompt "GICv3 ITS MSI controller support" if EXPERT = "y"
 depends on HAS_GICV3

+config VPL011_CONSOLE
+   bool "Emulated pl011 console support"
+   default y
+   ---help---
+ Allows a guest to use pl011 UART as a console
 endmenu

 menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..15efc13 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -52,6 +52,7 @@ obj-y += vm_event.o
 obj-y += vtimer.o
 obj-y += vpsci.o
 obj-y += vuart.o
+obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o

 #obj-bin-y += o

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
new file mode 100644
index 000..51abade
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,340 @@
+/*
+ * arch/arm/vpl011.c
+ *
+ * Virtual PL011 UART
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+unsigned int vpl011_reg_mask[] = {0xff, 0x, 0x};


This should be static. But I don't get what you need that. In the first 
version, I suggested to re-purpose vgic_reg*_{extract,update} so we can 
use it here. It would probably need to be renamed to vreg_reg*.


I don't see any reason to not do that and re-inventing the wheel.


+
+static void vgic_inject_vpl011_spi(struct domain *d)
+{
+struct vpl011_s *vpl011 = &d->arch.vpl011;
+
+if ( (vpl011->uartris & vpl011->uartimsc) )
+vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
+}


PL011 is using level interrupt which means that you should not inject 
interrupt but instead set or clear the pending interrupt.


However, the problem is because the vGIC is incapable to handle properly 
level interrupt. This is going to be a major problem as the interrupt 
should stay pending until the pl011 emulation says there are no more 
interrupts to handle.


For instance, you may miss character if the guest driver had not enough 
space to read character new one because the interrupt will not get 
re-injected.


I am not asking to modify the vGIC in order to handle level properly 
(Andre in CC is looking at that). But we need to get the code in correct 
shape in order to handle properly pl011 interrupt.


By that I mean, at least the naming of the function (I haven't read the 
rest to know what is missing). I.e I would rename to vpl011_update(...).



+static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, 
void *priv)
+{
+uint8_t ch;
+struct hsr_dabt dabt = info->dabt;
+int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
+struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
+
+switch ( vpl011_reg )
+{
+case DR:


As said on the first version, all those registers have specific size. 
Please have a look at how we handle register emulation in the vgic with 
VREG*

Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-05-05 Thread Bhupinder Thakur 
Hi Julien,

>> +
>> +unsigned int vpl011_reg_mask[] = {0xff, 0x, 0x};
>
>
> This should be static. But I don't get what you need that. In the first
> version, I suggested to re-purpose vgic_reg*_{extract,update} so we can use
> it here. It would probably need to be renamed to vreg_reg*.
>
> I don't see any reason to not do that and re-inventing the wheel.

I understand that the vreg_reg* functions are handy in scenarios where
user may want to access the data at some offset from the register base
address. The SBSA spec specifies that the base address of the access
must be same as the base address of the register. So in this case the
offset would always be 0. That is the reason I used a simple mask to
return 8-bit, 16-bit and 32-bit values.
>
>> +
>> +static void vgic_inject_vpl011_spi(struct domain *d)
>> +{
>> +struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +if ( (vpl011->uartris & vpl011->uartimsc) )
>> +vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>> +}
>
>
> PL011 is using level interrupt which means that you should not inject
> interrupt but instead set or clear the pending interrupt.
>
> However, the problem is because the vGIC is incapable to handle properly
> level interrupt. This is going to be a major problem as the interrupt should
> stay pending until the pl011 emulation says there are no more interrupts to
> handle.
>
> For instance, you may miss character if the guest driver had not enough
> space to read character new one because the interrupt will not get
> re-injected.
>
> I am not asking to modify the vGIC in order to handle level properly (Andre
> in CC is looking at that). But we need to get the code in correct shape in
> order to handle properly pl011 interrupt.
>
> By that I mean, at least the naming of the function (I haven't read the rest
> to know what is missing). I.e I would rename to vpl011_update(...).
Should I define two functions vgic_vpl011_spi_set() and
vgic_vpl011_spi_clear()? For now, I can keep vgic_vpl011_spi_clear()
empty and rename
vgic_inject_vpl011_spi() to vgic_vpl011_spi_set(). I will call
vgic_vpl011_spi_clear() at all places where IN ring buffer has become
empty.

>
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
>> *r, void *priv)
>> +{
>> +uint8_t ch;
>> +struct hsr_dabt dabt = info->dabt;
>> +int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>> +struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> +switch ( vpl011_reg )
>> +{
>> +case DR:
>
>
> As said on the first version, all those registers have specific size. Please
> have a look at how we handle register emulation in the vgic with VREG*.
Since SBSA specs mandate that pl011 register accesses must always be
accessed using the register base address, I am using the register base
address here instead of an address range.
>
>> +if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> +vpl011_read_data(v->domain, &ch);
>> +*r = ch;
>> +break;
>> +
>> +case RSR:
>> +if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> +
>> +/* It always returns 0 as there are no physical errors. */
>> +*r = 0;
>> +break;
>> +
>> +case FR:
>> +if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>> +*r = (vpl011->uartfr & vpl011_reg_mask[dabt.size]);
>
>
> Pointless ().
>
I will remove () at all relevant places.

>> +
>> +write_ignore:
>> +if ( !VALID_BW_SIZE(dabt.size) ) goto bad_width;
>
>
> Why? Looking at the spec, write-ignore register does not have specific
> access size.
I will check.

>
>> +return 1;
>> +
>> +word_write_ignore:
>> +if ( !VALID_W_SIZE(dabt.size) ) goto bad_width;
>> +return 1;
>> +
>> +bad_width:
>> +gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
>> +   dabt.size, dabt.reg, vpl011_reg);
>> +domain_crash_synchronous();
>> +return 0;
>> +
>> +}
>> +
>> +static const struct mmio_handler_ops vpl011_mmio_handler = {
>> +.read = vpl011_mmio_read,
>> +.write = vpl011_mmio_write,
>> +};
>> +
>> +int vpl011_map_guest_page(struct domain *d, unsigned long pfn)
>
>
> Please don't use the term pfn as we don't know whether it refers to the
> guest frame number (GFN) or machine frame number (MFN).
>
> So in this case, I think it is gfn. Also s/unsigned long/gfn_t/
>
ok.
>
>> +{
>> +struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +/* Map the guest PFN to Xen address space. */
>> +return prepare_ring_for_helper(d,
>> +   pfn,
>> +   &vpl011->ring_page,
>> +   &vpl011->ring_buf);
>> +}
>> +
>> +static void vpl011_data_avail(struct domain *d)
>> +{
>> +unsigned long flags;
>> +struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +struct xencons_interface *intf = vpl011->ring_buf;
>> +uint32_t in_ring_depth, out_ring_depth;
>> +
>> +VPL011_LOCK(d, flags);
>

Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-05-05 Thread Julien Grall 



On 05/05/17 12:18, Bhupinder Thakur wrote:

Hi Julien,


Hi Bhupinder,


+
+unsigned int vpl011_reg_mask[] = {0xff, 0x, 0x};



This should be static. But I don't get what you need that. In the first
version, I suggested to re-purpose vgic_reg*_{extract,update} so we can use
it here. It would probably need to be renamed to vreg_reg*.

I don't see any reason to not do that and re-inventing the wheel.


I understand that the vreg_reg* functions are handy in scenarios where
user may want to access the data at some offset from the register base
address. The SBSA spec specifies that the base address of the access
must be same as the base address of the register. So in this case the
offset would always be 0. That is the reason I used a simple mask to
return 8-bit, 16-bit and 32-bit values.


The part "The SBSA spec specifies that the base address of the 
access..." should have been specified in the commit message because 
reading at the PL011 spec, I don't see this limit.


The reason of using vreg_reg* is to have all MMIOs emulation using the 
same helpers and avoid everyone to re-invent the wheel because you can 
"optimize" for your case.


Also, it is also more obvious to read vreg_reg32_update(...) than "& 
vpl011_reg_mask[dabt.size]". This would avoid quite a lot rework if we 
ever decide to modify the reg emulation.





+
+static void vgic_inject_vpl011_spi(struct domain *d)
+{
+struct vpl011_s *vpl011 = &d->arch.vpl011;
+
+if ( (vpl011->uartris & vpl011->uartimsc) )
+vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
+}



PL011 is using level interrupt which means that you should not inject
interrupt but instead set or clear the pending interrupt.

However, the problem is because the vGIC is incapable to handle properly
level interrupt. This is going to be a major problem as the interrupt should
stay pending until the pl011 emulation says there are no more interrupts to
handle.

For instance, you may miss character if the guest driver had not enough
space to read character new one because the interrupt will not get
re-injected.

I am not asking to modify the vGIC in order to handle level properly (Andre
in CC is looking at that). But we need to get the code in correct shape in
order to handle properly pl011 interrupt.

By that I mean, at least the naming of the function (I haven't read the rest
to know what is missing). I.e I would rename to vpl011_update(...).

Should I define two functions vgic_vpl011_spi_set() and
vgic_vpl011_spi_clear()? For now, I can keep vgic_vpl011_spi_clear()
empty and rename
vgic_inject_vpl011_spi() to vgic_vpl011_spi_set(). I will call
vgic_vpl011_spi_clear() at all places where IN ring buffer has become
empty.


The code should only call a function vpl011_update that will clear or 
set the line. I don't see why it would need to specifically call clear/set.







+static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
*r, void *priv)
+{
+uint8_t ch;
+struct hsr_dabt dabt = info->dabt;
+int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
+struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
+
+switch ( vpl011_reg )
+{
+case DR:



As said on the first version, all those registers have specific size. Please
have a look at how we handle register emulation in the vgic with VREG*.

Since SBSA specs mandate that pl011 register accesses must always be
accessed using the register base address, I am using the register base
address here instead of an address range.


Then it should have been written in the commit message. I made this 
comment on the previous version and didn't see any answer from you. So I 
considered you forgot to address it.


A general rule is to answer on the review e-mail or specify after "---" 
why you didn't address a comment so we know why it has not been done. 
Reviewers may guess wrong why you didn't do it :).


[...]


Missing Emacs magic.

Can you please elaborate this comment?


All files in Xen contains the below lines to help e-macs to load the 
correct format:


/*
 * Local variables:
 * mode: C
 * c-file-style: "BSD"
 * c-basic-offset: 4
 * indent-tabs-mode: nil
 * End:
 */

This should be added on any new files using Xen coding style.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation in Xen

2017-05-05 Thread Bhupinder Thakur 
Hi Julien,

 +unsigned int vpl011_reg_mask[] = {0xff, 0x, 0x};
>>>
>>>
>>>
>>> This should be static. But I don't get what you need that. In the first
>>> version, I suggested to re-purpose vgic_reg*_{extract,update} so we can
>>> use
>>> it here. It would probably need to be renamed to vreg_reg*.
>>>
>>> I don't see any reason to not do that and re-inventing the wheel.
>>
>>
>> I understand that the vreg_reg* functions are handy in scenarios where
>> user may want to access the data at some offset from the register base
>> address. The SBSA spec specifies that the base address of the access
>> must be same as the base address of the register. So in this case the
>> offset would always be 0. That is the reason I used a simple mask to
>> return 8-bit, 16-bit and 32-bit values.
>
>
> The part "The SBSA spec specifies that the base address of the access..."
> should have been specified in the commit message because reading at the
> PL011 spec, I don't see this limit.
>
> The reason of using vreg_reg* is to have all MMIOs emulation using the same
> helpers and avoid everyone to re-invent the wheel because you can "optimize"
> for your case.
>
> Also, it is also more obvious to read vreg_reg32_update(...) than "&
> vpl011_reg_mask[dabt.size]". This would avoid quite a lot rework if we ever
> decide to modify the reg emulation.
>
ok. I will redefine the vgic_reg* functions to generic vreg_reg* and
move the definitions to vreg.h, so that those can be used by vpl011
also.
+
>
>>>
 +
 +static void vgic_inject_vpl011_spi(struct domain *d)
 +{
 +struct vpl011_s *vpl011 = &d->arch.vpl011;
 +
 +if ( (vpl011->uartris & vpl011->uartimsc) )
 +vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
 +}
>>>
>>>
>>>
>>> PL011 is using level interrupt which means that you should not inject
>>> interrupt but instead set or clear the pending interrupt.
>>>
>>> However, the problem is because the vGIC is incapable to handle properly
>>> level interrupt. This is going to be a major problem as the interrupt
>>> should
>>> stay pending until the pl011 emulation says there are no more interrupts
>>> to
>>> handle.
>>>
>>> For instance, you may miss character if the guest driver had not enough
>>> space to read character new one because the interrupt will not get
>>> re-injected.
>>>
>>> I am not asking to modify the vGIC in order to handle level properly
>>> (Andre
>>> in CC is looking at that). But we need to get the code in correct shape
>>> in
>>> order to handle properly pl011 interrupt.
>>>
>>> By that I mean, at least the naming of the function (I haven't read the
>>> rest
>>> to know what is missing). I.e I would rename to vpl011_update(...).
>>
>> Should I define two functions vgic_vpl011_spi_set() and
>> vgic_vpl011_spi_clear()? For now, I can keep vgic_vpl011_spi_clear()
>> empty and rename
>> vgic_inject_vpl011_spi() to vgic_vpl011_spi_set(). I will call
>> vgic_vpl011_spi_clear() at all places where IN ring buffer has become
>> empty.
>
>
> The code should only call a function vpl011_update that will clear or set
> the line. I don't see why it would need to specifically call clear/set.
>
ok. I will rename vgic_inject_vpl011_spi() to vpl011_update_irq().

>>
>>>
 +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info,
 register_t
 *r, void *priv)
 +{
 +uint8_t ch;
 +struct hsr_dabt dabt = info->dabt;
 +int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
 +struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
 +
 +switch ( vpl011_reg )
 +{
 +case DR:
>>>
>>>
>>>
>>> As said on the first version, all those registers have specific size.
>>> Please
>>> have a look at how we handle register emulation in the vgic with VREG*.
>>
>> Since SBSA specs mandate that pl011 register accesses must always be
>> accessed using the register base address, I am using the register base
>> address here instead of an address range.
>
>
> Then it should have been written in the commit message. I made this comment
> on the previous version and didn't see any answer from you. So I considered
> you forgot to address it.
>
> A general rule is to answer on the review e-mail or specify after "---" why
> you didn't address a comment so we know why it has not been done. Reviewers
> may guess wrong why you didn't do it :).
ok. I will update the commit message accordingly.

>
> [...]
>
>>> Missing Emacs magic.
>>
>> Can you please elaborate this comment?
>
>
> All files in Xen contains the below lines to help e-macs to load the correct
> format:
>
> /*
>  * Local variables:
>  * mode: C
>  * c-file-style: "BSD"
>  * c-basic-offset: 4
>  * indent-tabs-mode: nil
>  * End:
>  */>
> This should be added on any new files using Xen coding style.
>
Thanks. I will add this to the new files.

Regards,
Bhupinder

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