Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall]

Hi,

On 19/06/17 11:14, Andre Przywara wrote:

+
+static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
+{
+return (dabt.size != DABT_DOUBLE_WORD);
+}
+
+static void vpl011_update(struct domain *d)


Can you please rename this function to indicate that it updates the
*interrupt status*? That name as it is rather generic at the moment.


Well, it updates the pl011 state even though today it is only handling 
interrupt...



+static int vpl011_mmio_write(struct vcpu *v,
+ mmio_info_t *info,
+ register_t r,
+ void *priv)
+{
+struct hsr_dabt dabt = info->dabt;
+uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+struct vpl011 *vpl011 = >domain->arch.vpl011;
+struct domain *d = v->domain;
+unsigned long flags;
+
+switch ( vpl011_reg )
+{
+case DR:
+{
+uint32_t data = 0;
+
+/*
+ * Since pl011 registers are 32-bit registers, all registers
+ * are handled similarly allowing 8-bit, 16-bit and 32-bit
+ * accesses.
+ */
+if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+vreg_reg32_update(, r, info);
+data &= 0xFF;


This is not needed as vpl011_write_data()'s second argument is uint8_t,
so the compiler does this masking anyway.
And even if it wouldn't, you could move this statement into the call.


Sorry, I have only spotted this comment now. We should really avoid 
implicit cast when calling function. This is a call to make error if we 
ever decide to change the type of the parameter. More than the local 
variable data is 32-bit.


Better to play safe than handling security issue after afterwards.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Bhupinder Thakur]

Hi Andre,

Thanks for your comments.

>> 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
>
> I admit that I am rather late with this comment, but I am not sure we
> should advertise PL011 emulation here.
> Technically what you emulate is a "SBSA Generic UART", which is indeed a
> subset of the PL011, but really only a subset. You confirm this already
> in patch 13/14, where you use the respective compatible name instead of
> the generic arm,pl011 one.
>
> So while I don't dare to ask for renaming every identifier, I wonder if
> we should at least keep the publicly visible part confined to "SBSA
> UART". You could mention the subset nature in the help message here, for
> instance.
> The same reasoning applies to other parts of this series which introduce
> user-visible strings (like in libxl).
>
>>  endmenu
>>
I will rename the user visible part to "sbsa_uart".

>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>
> I think Julien mentioned this already, but I think you should just use
> "#include " here.
>
ok.
>> +
>> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
>> +{
>> +return (dabt.size != DABT_DOUBLE_WORD);
>> +}
>> +
>> +static void vpl011_update(struct domain *d)
>
> Can you please rename this function to indicate that it updates the
> *interrupt status*? That name as it is rather generic at the moment.
>
I will rename it to vpl011_update_interrupt_status.

>> +{
>> +struct vpl011 *vpl011 = >arch.vpl011;
>> +
>> +/*
>> + * TODO: PL011 interrupts are level triggered which means
>> + * that interrupt needs to be set/clear instead of being
>> + * injected. However, currently vGIC does not handle level
>> + * triggered interrupts properly. This function needs to be
>> + * revisited once vGIC starts handling level triggered
>> + * interrupts.
>> + */
>> +if ( vpl011->uartris & vpl011->uartimsc )
>> +vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>
> So my understanding is that this is using an edge triggered semantic at
> the moment. While this is not what an PL011 actually implements and not
> what the driver really expects, this is fine on itself for now.
> BUT I think we may want to avoid injecting spurious interrupts now:
> If for instance the receive interrupt condition is set and we clear the
> transmit interrupt bit, then call this function, it would inject a new
> interrupt, although in a edge-triggered world we really wouldn't need to
> do anything.
> So I believe we would need to have some kind of shadowed interrupt
> state, which stores the interrupt condition the guest already knows
> about. As soon as we *add* a bit to this state, we inject the SPI.
> This would act as a kind of temporary bridge between the level triggered
> SBSA/PL011 UART and the edge-only VGIC implementation for now.
> Later when the VGIC properly handles level triggered interrupts, this
> implementation can be adjusted. But this change should then be confined
> to this very function.
>
ok. I will update the logic accordingly.

>> +}
>> +
>> +static uint8_t vpl011_read_data(struct domain *d)
>> +{
>> +unsigned long flags;
>> +uint8_t data = 0;
>> +struct vpl011 *vpl011 = >arch.vpl011;
>> +struct xencons_interface *intf = vpl011->ring_buf;
>> +XENCONS_RING_IDX in_cons = intf->in_cons;
>> +XENCONS_RING_IDX in_prod = intf->in_prod;
>> +
>> +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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>> +{
>> +data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>> +in_cons += 1;
>> +intf->in_cons = in_cons;
>> +smp_mb();
>> +}
>> +else
>> +{
>> +gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>> +}
>> +
>> +if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
>> +{
>> +vpl011->uartfr |= RXFE;
>> +vpl011->uartris &= ~RXI;
>
> In a level triggered world you would need to possibly change the status
> of the interrupt line here, so a call to (a renamed and fixed)
> vpl011_update() function would be due here. To make the transition later
> as easy 

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Andre Przywara]

Hi,

On 06/06/17 18:25, 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 
> ---
> CC: ij
> CC: wl
> CC: ss
> CC: jg
> CC: kw
> 
> Changes since v3:
> - Moved the call to DEFINE_XEN_FLEX_RING from vpl011.h to public/console.h. 
> This macro defines
>   standard functions to operate on the ring buffer.
> - Lock taken while updating the interrupt mask and clear registers in 
> mmio_write.
> - Use gfn_t instead of xen_pfn_t.
> - vgic_free_virq called if there is any error in vpl011 initialization.
> - mmio handlers freed if there is any error in vpl011 initialization.
> - Removed vpl011->initialized flag usage as the same check could be done 
>   using vpl011->ring-ref.
> - Used return instead of break in the switch handling of emulation of 
> different pl011 registers.
> - Renamed vpl011_update_spi() to vpl011_update().
> 
> Changes since v2:
> - Use generic vreg_reg* for read/write of registers emulating pl011.
> - Use generic ring buffer functions defined using DEFINE_XEN_FLEX_RING.
> - Renamed the SPI injection function to vpl011_update_spi() to reflect level 
>   triggered nature of pl011 interrupts.
> - The pl011 register access address should always be the base address of the
>   corresponding register as per section B of the SBSA document. For this 
> reason,
>   the register range address access is not allowed.
> 
> Changes since v1:
> - Removed the optimiztion related to sendiing events to xenconsole 
> - Use local variables as ring buffer indices while using the ring buffer
> 
>  tools/console/daemon/io.c|   2 +-
>  xen/arch/arm/Kconfig |   5 +
>  xen/arch/arm/Makefile|   1 +
>  xen/arch/arm/vpl011.c| 418 
> +++
>  xen/include/asm-arm/domain.h |   6 +
>  xen/include/asm-arm/pl011-uart.h |   2 +
>  xen/include/asm-arm/vpl011.h |  74 +++
>  xen/include/public/arch-arm.h|   6 +
>  xen/include/public/io/console.h  |   4 +
>  9 files changed, 517 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/vpl011.c
>  create mode 100644 xen/include/asm-arm/vpl011.h
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e6a886..947f13a 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -21,6 +21,7 @@
>  
>  #include "utils.h"
>  #include "io.h"
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -29,7 +30,6 @@
>  
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> 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

I admit that I am rather late with this comment, but I am not sure we
should advertise PL011 emulation here.
Technically what you emulate is a "SBSA Generic UART", which is indeed a
subset of the PL011, but really only a subset. You confirm this already
in patch 13/14, where you use the respective compatible name instead of
the generic arm,pl011 one.

So while I don't dare to ask for renaming every identifier, I wonder if
we should at least keep the publicly visible part confined to "SBSA
UART". You could mention the subset nature in the help message here, for
instance.
The same reasoning applies to other parts of this series which introduce
user-visible strings (like in libxl).

>  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..9b1f27e
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
> @@ -0,0 +1,418 @@
> +/*
> + * arch/arm/vpl011.c
> + *
> + * Virtual PL011 UART
> + *
> + * 

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall]

On 06/14/2017 06:47 AM, Bhupinder Thakur wrote:

Hi Julien,


Hi Bhupinder,




+}
+
+static void vpl011_update(struct domain *d)
+{
+struct vpl011 *vpl011 = >arch.vpl011;
+
+/*
+ * TODO: PL011 interrupts are level triggered which means
+ * that interrupt needs to be set/clear instead of being
+ * injected. However, currently vGIC does not handle level
+ * triggered interrupts properly. This function needs to be
+ * revisited once vGIC starts handling level triggered
+ * interrupts.
+ */
+if ( vpl011->uartris & vpl011->uartimsc )




The write in uartirs and uartimsc are protected by a lock. Shouldn't it
be
the case here too? More that they are not updated atomically.

You probably want to call vpl011_update with vpl011 lock taken to make
sure
you don't have any synchronization issue.



Since we are just reading the values here, I think it is fine to not
take a lock. The
condition will either be true or false.



uartris and uartimsc may not be updated atomically because vreg_reg32_update
does not guarantee it.

So you may read a wrong value here and potentially inject spurious
interrupt. This will get much worse when level will fully be supported as
you may switch the level of the interrupt by mistake.


Ok. I will check this condition under lock. But should I call
vgic_vcpu_inject_spi() also under the same lock? I think we can do
like this:

vpl011_lock();
mask =  vpl011->uartris & vpl011->uartimsc;
vpl011_unlock();

if ( mask )
vgic_vcpu_inject_spi();


This is not going to work the day we rework the vGIC to fully support 
level interrupt. If you don't protect vgic_vcpu_inject_spi(), you may 
lower the level by mistake.


As mentioned on a previous mail, I would prefer if vpl011_update is 
called with the lock taken.


[...]


How do you know the compiler will generate assembly which contain the data
dependency?

Likely it will not be the case because in_cons will be used indirectly as we
mask it first.


The memory barrier
after the 3 statements ensures that intf->in_cons is updated before
proceeding ahead.



Can you explain why?

IHMO, what matter here is in_cons to be written after intf->in[...] is read.
Otherwise the backend may see in_cons before the character has effectively
been read.


ok. The issue is that the other end (xenconsole running on maybe some
other CPU) may see the increment operation first before the read
operation could complete (due to some quirk of memory/cache
architecture) even though the CPU running the mmio_read() will see the
effect in the correct order only.


It is not about quirk of memory/cache but data access ordering at the 
processor level. A processor is free to re-order the access if it is 
more efficient. That's why you have the mb/smp_mb barrier to tell the 
processor to no do that.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Bhupinder Thakur]

Hi Julien,


 +}
 +
 +static void vpl011_update(struct domain *d)
 +{
 +struct vpl011 *vpl011 = >arch.vpl011;
 +
 +/*
 + * TODO: PL011 interrupts are level triggered which means
 + * that interrupt needs to be set/clear instead of being
 + * injected. However, currently vGIC does not handle level
 + * triggered interrupts properly. This function needs to be
 + * revisited once vGIC starts handling level triggered
 + * interrupts.
 + */
 +if ( vpl011->uartris & vpl011->uartimsc )
>>>
>>>
>>>
>>> The write in uartirs and uartimsc are protected by a lock. Shouldn't it
>>> be
>>> the case here too? More that they are not updated atomically.
>>>
>>> You probably want to call vpl011_update with vpl011 lock taken to make
>>> sure
>>> you don't have any synchronization issue.
>>
>>
>> Since we are just reading the values here, I think it is fine to not
>> take a lock. The
>> condition will either be true or false.
>
>
> uartris and uartimsc may not be updated atomically because vreg_reg32_update
> does not guarantee it.
>
> So you may read a wrong value here and potentially inject spurious
> interrupt. This will get much worse when level will fully be supported as
> you may switch the level of the interrupt by mistake.
>
Ok. I will check this condition under lock. But should I call
vgic_vcpu_inject_spi() also under the same lock? I think we can do
like this:

vpl011_lock();
mask =  vpl011->uartris & vpl011->uartimsc;
vpl011_unlock();

if ( mask )
   vgic_vcpu_inject_spi();
>

>>> See my answer on Stefano's e-mail regarding the barrier here.
>>> ()
>>>
 +
 +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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
 +{
 +data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
 +in_cons += 1;
 +intf->in_cons = in_cons;
 +smp_mb();
>>>
>>>
>>>
>>> I don't understand why you moved the barrier from between reading the
>>> data
>>> and intf->in_cons. You have to ensure the character is read before
>>> updating
>>> in_cons.
>>
>> I thought that since these 3 statements are dependent on in_cons, they
>> would be executed in order due to data dependency.
>
>
> How do you know the compiler will generate assembly which contain the data
> dependency?
>
> Likely it will not be the case because in_cons will be used indirectly as we
> mask it first.
>
>> The memory barrier
>> after the 3 statements ensures that intf->in_cons is updated before
>> proceeding ahead.
>
>
> Can you explain why?
>
> IHMO, what matter here is in_cons to be written after intf->in[...] is read.
> Otherwise the backend may see in_cons before the character has effectively
> been read.
>
ok. The issue is that the other end (xenconsole running on maybe some
other CPU) may see the increment operation first before the read
operation could complete (due to some quirk of memory/cache
architecture) even though the CPU running the mmio_read() will see the
effect in the correct order only.

I will move the smp_mb() before index is incremented.

>>> What if the other end of the ring has put more data whilst reading one
>>> character?
>>
>> It will raise an event when the other end puts more data and in the
>> event handling function data_available(), it will clear the RXFE bit.
>
>
> And this is fine because the lock is here to protect uartfr/uartis I guess?
Yes.

>
> [...]
>
 +
 +/* Map the guest PFN to Xen address space. */
 +rc =  prepare_ring_for_helper(d,
 +  gfn_x(info->gfn),
 +  >ring_page,
 +  >ring_buf);
 +if ( rc < 0 )
 +goto out;
 +
 +rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
 +if ( !rc )
 +{
 +rc = -EINVAL;
 +goto out1;
 +}
 +
 +register_mmio_handler(d, _mmio_handler,
 +  GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
>>>
>>>
>>>
>>> Again, you register MMIO handler but never remove them. So if this call
>>> fail, you will end up with the handlers existing but the rest
>>> half-initialized which likely lead to a segfault, or worst leaking data.
>>
>> This function does not return a status. So there is no way to find out
>> if the mmio handlers were
>> registered successfully.
>
>
> That's not my point. register_mmio_handler should never fail. However, the
> code below (alloc_unbount_...) may fail. And you bail
>
>> I am 

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Stefano Stabellini]

On Tue, 13 Jun 2017, Julien Grall wrote:
> > > >  tools/console/daemon/io.c|   2 +-
> > > >  xen/arch/arm/Kconfig |   5 +
> > > >  xen/arch/arm/Makefile|   1 +
> > > >  xen/arch/arm/vpl011.c| 418
> > > > +++
> > > >  xen/include/asm-arm/domain.h |   6 +
> > > >  xen/include/asm-arm/pl011-uart.h |   2 +
> > > >  xen/include/asm-arm/vpl011.h |  74 +++
> > > >  xen/include/public/arch-arm.h|   6 +
> > > >  xen/include/public/io/console.h  |   4 +
> > > 
> > > 
> > > This would require an ACK from Konrad. The addition would also need to be
> > > justified in the commit message. Although, you probably want to split this
> > > change in a separate patch.
> > I will send this change in a separate patch.
> > 
> > > 
> > > >  9 files changed, 517 insertions(+), 1 deletion(-)
> > > >  create mode 100644 xen/arch/arm/vpl011.c
> > > >  create mode 100644 xen/include/asm-arm/vpl011.h
> > > > 
> > > > diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> > > > index 7e6a886..947f13a 100644
> > > > --- a/tools/console/daemon/io.c
> > > > +++ b/tools/console/daemon/io.c
> > > 
> > > 
> > > Can you explain why you change the position of the include in io.c?
> > Since I am including ring.h in console.h, it needs string.h to be
> > included first.
> 
> This should be justified in the commit message.
> 
> [...]
> 
> > > > +}
> > > > +
> > > > +static void vpl011_update(struct domain *d)
> > > > +{
> > > > +struct vpl011 *vpl011 = >arch.vpl011;
> > > > +
> > > > +/*
> > > > + * TODO: PL011 interrupts are level triggered which means
> > > > + * that interrupt needs to be set/clear instead of being
> > > > + * injected. However, currently vGIC does not handle level
> > > > + * triggered interrupts properly. This function needs to be
> > > > + * revisited once vGIC starts handling level triggered
> > > > + * interrupts.
> > > > + */
> > > > +if ( vpl011->uartris & vpl011->uartimsc )
> > > 
> > > 
> > > The write in uartirs and uartimsc are protected by a lock. Shouldn't it be
> > > the case here too? More that they are not updated atomically.
> > > 
> > > You probably want to call vpl011_update with vpl011 lock taken to make
> > > sure
> > > you don't have any synchronization issue.
> > 
> > Since we are just reading the values here, I think it is fine to not
> > take a lock. The
> > condition will either be true or false.
> 
> uartris and uartimsc may not be updated atomically because vreg_reg32_update
> does not guarantee it.
> 
> So you may read a wrong value here and potentially inject spurious interrupt.
> This will get much worse when level will fully be supported as you may switch
> the level of the interrupt by mistake.
> 
> > 
> > > 
> > > > +vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> > > > +}
> > > > +
> > > > +static uint8_t vpl011_read_data(struct domain *d)
> > > > +{
> > > > +unsigned long flags;
> > > > +uint8_t data = 0;
> > > > +struct vpl011 *vpl011 = >arch.vpl011;
> > > > +struct xencons_interface *intf = vpl011->ring_buf;
> > > > +XENCONS_RING_IDX in_cons = intf->in_cons;
> > > > +XENCONS_RING_IDX in_prod = intf->in_prod;
> > > 
> > > 
> > > See my answer on Stefano's e-mail regarding the barrier here.
> > > ()
> > > 
> > > > +
> > > > +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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
> > > > +{
> > > > +data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
> > > > +in_cons += 1;
> > > > +intf->in_cons = in_cons;
> > > > +smp_mb();
> > > 
> > > 
> > > I don't understand why you moved the barrier from between reading the data
> > > and intf->in_cons. You have to ensure the character is read before
> > > updating
> > > in_cons.
> > I thought that since these 3 statements are dependent on in_cons, they
> > would be executed in order due to data dependency.
> 
> How do you know the compiler will generate assembly which contain the data
> dependency?
> 
> Likely it will not be the case because in_cons will be used indirectly as we
> mask it first.
> 
> > The memory barrier
> > after the 3 statements ensures that intf->in_cons is updated before
> > proceeding ahead.
> 
> Can you explain why?
> 
> IHMO, what matter here is in_cons to be written after intf->in[...] is read.
> Otherwise the backend may see in_cons before the character has effectively
> been read.

Julien is right, I missed it in my review. The smp_mb() should 

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall]

On 13/06/17 11:57, Bhupinder Thakur wrote:

Hi Julien,


Hi Bhupinder,





 tools/console/daemon/io.c|   2 +-
 xen/arch/arm/Kconfig |   5 +
 xen/arch/arm/Makefile|   1 +
 xen/arch/arm/vpl011.c| 418
+++
 xen/include/asm-arm/domain.h |   6 +
 xen/include/asm-arm/pl011-uart.h |   2 +
 xen/include/asm-arm/vpl011.h |  74 +++
 xen/include/public/arch-arm.h|   6 +
 xen/include/public/io/console.h  |   4 +



This would require an ACK from Konrad. The addition would also need to be
justified in the commit message. Although, you probably want to split this
change in a separate patch.

I will send this change in a separate patch.




 9 files changed, 517 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/asm-arm/vpl011.h

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..947f13a 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c



Can you explain why you change the position of the include in io.c?

Since I am including ring.h in console.h, it needs string.h to be
included first.


This should be justified in the commit message.

[...]


+}
+
+static void vpl011_update(struct domain *d)
+{
+struct vpl011 *vpl011 = >arch.vpl011;
+
+/*
+ * TODO: PL011 interrupts are level triggered which means
+ * that interrupt needs to be set/clear instead of being
+ * injected. However, currently vGIC does not handle level
+ * triggered interrupts properly. This function needs to be
+ * revisited once vGIC starts handling level triggered
+ * interrupts.
+ */
+if ( vpl011->uartris & vpl011->uartimsc )



The write in uartirs and uartimsc are protected by a lock. Shouldn't it be
the case here too? More that they are not updated atomically.

You probably want to call vpl011_update with vpl011 lock taken to make sure
you don't have any synchronization issue.


Since we are just reading the values here, I think it is fine to not
take a lock. The
condition will either be true or false.


uartris and uartimsc may not be updated atomically because 
vreg_reg32_update does not guarantee it.


So you may read a wrong value here and potentially inject spurious 
interrupt. This will get much worse when level will fully be supported 
as you may switch the level of the interrupt by mistake.







+vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
+}
+
+static uint8_t vpl011_read_data(struct domain *d)
+{
+unsigned long flags;
+uint8_t data = 0;
+struct vpl011 *vpl011 = >arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+XENCONS_RING_IDX in_cons = intf->in_cons;
+XENCONS_RING_IDX in_prod = intf->in_prod;



See my answer on Stefano's e-mail regarding the barrier here.
()


+
+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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
+{
+data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
+in_cons += 1;
+intf->in_cons = in_cons;
+smp_mb();



I don't understand why you moved the barrier from between reading the data
and intf->in_cons. You have to ensure the character is read before updating
in_cons.

I thought that since these 3 statements are dependent on in_cons, they
would be executed in order due to data dependency.


How do you know the compiler will generate assembly which contain the 
data dependency?


Likely it will not be the case because in_cons will be used indirectly 
as we mask it first.



The memory barrier
after the 3 statements ensures that intf->in_cons is updated before
proceeding ahead.


Can you explain why?

IHMO, what matter here is in_cons to be written after intf->in[...] is 
read. Otherwise the backend may see in_cons before the character has 
effectively been read.



What if the other end of the ring has put more data whilst reading one
character?

It will raise an event when the other end puts more data and in the
event handling function data_available(), it will clear the RXFE bit.


And this is fine because the lock is here to protect uartfr/uartis I guess?

[...]


+
+/* Map the guest PFN to Xen address space. */
+rc =  prepare_ring_for_helper(d,
+  gfn_x(info->gfn),
+  >ring_page,
+  >ring_buf);
+if ( rc < 0 )
+goto out;
+
+rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
+if ( !rc )
+{
+rc = -EINVAL;
+goto out1;
+}
+
+register_mmio_handler(d, _mmio_handler,
+ 

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Bhupinder Thakur]

Hi Julien,


>>
>>  tools/console/daemon/io.c|   2 +-
>>  xen/arch/arm/Kconfig |   5 +
>>  xen/arch/arm/Makefile|   1 +
>>  xen/arch/arm/vpl011.c| 418
>> +++
>>  xen/include/asm-arm/domain.h |   6 +
>>  xen/include/asm-arm/pl011-uart.h |   2 +
>>  xen/include/asm-arm/vpl011.h |  74 +++
>>  xen/include/public/arch-arm.h|   6 +
>>  xen/include/public/io/console.h  |   4 +
>
>
> This would require an ACK from Konrad. The addition would also need to be
> justified in the commit message. Although, you probably want to split this
> change in a separate patch.
I will send this change in a separate patch.

>
>>  9 files changed, 517 insertions(+), 1 deletion(-)
>>  create mode 100644 xen/arch/arm/vpl011.c
>>  create mode 100644 xen/include/asm-arm/vpl011.h
>>
>> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
>> index 7e6a886..947f13a 100644
>> --- a/tools/console/daemon/io.c
>> +++ b/tools/console/daemon/io.c
>
>
> Can you explain why you change the position of the include in io.c?
Since I am including ring.h in console.h, it needs string.h to be
included first.

>
>> @@ -21,6 +21,7 @@
>>
>>  #include "utils.h"
>>  #include "io.h"
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -29,7 +30,6 @@
>>
>>  #include 
>>  #include 
>> -#include 
>>  #include 
>>  #include 
>>  #include 
>
>
>
> [...]
>
>>  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
>
>
> Please the alphabetical order. Just noticed vtimer is not correctly
> positioned. I will send a patch for that.
>
ok.
>
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
>> +{
>> +return (dabt.size != DABT_DOUBLE_WORD);
>
>
> Again, please add a comment explaining why we allow all the sizes but
> 64-bit.
>
>> +}
>> +
>> +static void vpl011_update(struct domain *d)
>> +{
>> +struct vpl011 *vpl011 = >arch.vpl011;
>> +
>> +/*
>> + * TODO: PL011 interrupts are level triggered which means
>> + * that interrupt needs to be set/clear instead of being
>> + * injected. However, currently vGIC does not handle level
>> + * triggered interrupts properly. This function needs to be
>> + * revisited once vGIC starts handling level triggered
>> + * interrupts.
>> + */
>> +if ( vpl011->uartris & vpl011->uartimsc )
>
>
> The write in uartirs and uartimsc are protected by a lock. Shouldn't it be
> the case here too? More that they are not updated atomically.
>
> You probably want to call vpl011_update with vpl011 lock taken to make sure
> you don't have any synchronization issue.

Since we are just reading the values here, I think it is fine to not
take a lock. The
condition will either be true or false.

>
>> +vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>> +}
>> +
>> +static uint8_t vpl011_read_data(struct domain *d)
>> +{
>> +unsigned long flags;
>> +uint8_t data = 0;
>> +struct vpl011 *vpl011 = >arch.vpl011;
>> +struct xencons_interface *intf = vpl011->ring_buf;
>> +XENCONS_RING_IDX in_cons = intf->in_cons;
>> +XENCONS_RING_IDX in_prod = intf->in_prod;
>
>
> See my answer on Stefano's e-mail regarding the barrier here.
> ()
>
>> +
>> +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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>> +{
>> +data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>> +in_cons += 1;
>> +intf->in_cons = in_cons;
>> +smp_mb();
>
>
> I don't understand why you moved the barrier from between reading the data
> and intf->in_cons. You have to ensure the character is read before updating
> in_cons.
I thought that since these 3 statements are dependent on in_cons, they
would be executed in order due to data dependency. The memory barrier
after the 3 statements ensures that intf->in_cons is updated before
proceeding ahead.

>
>> +}
>> +else
>> +{
>> +gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>> +}
>
>
> The {} are not necessary.
ok.
>
>> +
>> +if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
>

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall]

On 13/06/2017 09:58, Bhupinder Thakur wrote:

Hi Julien,


Hi Bhupinder,


+static uint8_t vpl011_read_data(struct domain *d)
+{
+unsigned long flags;
+uint8_t data = 0;
+struct vpl011 *vpl011 = >arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+XENCONS_RING_IDX in_cons = intf->in_cons;
+XENCONS_RING_IDX in_prod = intf->in_prod;



After reading the indexes, we always need barriers. In this case:

  smp_rmb();



Well, there are already barrier with the spinlock. However, I am bit concern
about reading those index without the lock taken. You can have concurrent
call to vpl011_read_data happening and therefore the indexes may have
changed when the lock will be taken.


Is there a possibility of concurrent access since this function is
executed as part of
trap handling which will serialize access to this function?


There are no locking in the common trap handling. The emulation should 
take care of the locking because multiple vCPU can concurrently access 
the MMIO region.


[]


I am pretty sure that the PV console protocol requires us to notify the
other end even on reads. We need to add a notify_via_xen_event_channel
here, I think.



I would agree here. On the previous version, I asked Bhupinder to explain
why it is necessary and he said: "On second thoughs, notification is not
required".


I understand that xenconsole is currently using the event notification
as an indication to read
data from the ring buffer. For writing data, it keeps checking
periodically if there is space in the
ring buffer. If there is space then it writes more data.


You should not assume that xenconsoled is the only backend console. One 
could decide to implement its own.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Bhupinder Thakur]

Hi Julien,


>>> +static uint8_t vpl011_read_data(struct domain *d)
>>> +{
>>> +unsigned long flags;
>>> +uint8_t data = 0;
>>> +struct vpl011 *vpl011 = >arch.vpl011;
>>> +struct xencons_interface *intf = vpl011->ring_buf;
>>> +XENCONS_RING_IDX in_cons = intf->in_cons;
>>> +XENCONS_RING_IDX in_prod = intf->in_prod;
>>
>>
>> After reading the indexes, we always need barriers. In this case:
>>
>>   smp_rmb();
>
>
> Well, there are already barrier with the spinlock. However, I am bit concern
> about reading those index without the lock taken. You can have concurrent
> call to vpl011_read_data happening and therefore the indexes may have
> changed when the lock will be taken.

Is there a possibility of concurrent access since this function is
executed as part of
trap handling which will serialize access to this function?
>
>
>>
>>
>>> +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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>>> +{
>>> +data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>>> +in_cons += 1;
>>> +intf->in_cons = in_cons;
>>> +smp_mb();
>>> +}
>>> +else
>>> +{
>>> +gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer
>>> empty\n");
>>> +}
>>> +
>>> +if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
>>> +{
>>> +vpl011->uartfr |= RXFE;
>>> +vpl011->uartris &= ~RXI;
>>> +}
>>> +vpl011->uartfr &= ~RXFF;
>>> +VPL011_UNLOCK(d, flags);
>>
>>
>> I am pretty sure that the PV console protocol requires us to notify the
>> other end even on reads. We need to add a notify_via_xen_event_channel
>> here, I think.
>
>
> I would agree here. On the previous version, I asked Bhupinder to explain
> why it is necessary and he said: "On second thoughs, notification is not
> required".
>
I understand that xenconsole is currently using the event notification
as an indication to read
data from the ring buffer. For writing data, it keeps checking
periodically if there is space in the
ring buffer. If there is space then it writes more data.

I agree that as a protocol, it may be a requirement to send the
notifications on read also. I will
add back the notification for this case.

>>
>>
>>> +return data;
>>> +}
>>> +
>>> +static void vpl011_write_data(struct domain *d, uint8_t data)
>>> +{
>>> +unsigned long flags;
>>> +struct vpl011 *vpl011 = >arch.vpl011;
>>> +struct xencons_interface *intf = vpl011->ring_buf;
>>> +XENCONS_RING_IDX out_cons = intf->out_cons;
>>> +XENCONS_RING_IDX out_prod = intf->out_prod;
>>
>>
>>   smp_mb()
>
>
> Same remark as above.
>
> [...]
>
>>> +static void vpl011_data_avail(struct domain *d)
>>> +{
>>> +unsigned long flags;
>>> +struct vpl011 *vpl011 = >arch.vpl011;
>>> +struct xencons_interface *intf = vpl011->ring_buf;
>>> +XENCONS_RING_IDX in_cons = intf->in_cons;
>>> +XENCONS_RING_IDX in_prod = intf->in_prod;
>>> +XENCONS_RING_IDX out_cons = intf->out_cons;
>>> +XENCONS_RING_IDX out_prod = intf->out_prod;
>>> +XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
>>
>>
>>   smb_mb()
>
>
> Ditto.
>
Regards,
Bhupinder

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

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Stefano Stabellini]

On Fri, 9 Jun 2017, Julien Grall wrote:
> Hi,
> 
> On 07/06/17 00:02, Stefano Stabellini wrote:
> > On Tue, 6 Jun 2017, Bhupinder Thakur wrote:
> > > +static uint8_t vpl011_read_data(struct domain *d)
> > > +{
> > > +unsigned long flags;
> > > +uint8_t data = 0;
> > > +struct vpl011 *vpl011 = >arch.vpl011;
> > > +struct xencons_interface *intf = vpl011->ring_buf;
> > > +XENCONS_RING_IDX in_cons = intf->in_cons;
> > > +XENCONS_RING_IDX in_prod = intf->in_prod;
> > 
> > After reading the indexes, we always need barriers. In this case:
> > 
> >   smp_rmb();
> 
> Well, there are already barrier with the spinlock. However, I am bit concern
> about reading those index without the lock taken. You can have concurrent call
> to vpl011_read_data happening and therefore the indexes may have changed when
> the lock will be taken.

I don't like to rely on the barriers within spin_lock, which might or
might not change in future implementations, for PV protocols operations,
which need explicit barrier. But it is true the code would work today
from that perspective.

You are also right that with the current implementation vpl011_read_data
(and vpl011_write_data) could be called twice simultaneously. For that
to work correctly, we have to move the indexes reads after VPL011_LOCK.
In addition, we also need the explicit memory barrier

  smp_rmb()

after the indexes reads to be in sync with the other end. In other words:

  VPL011_LOCK(d, flags);

  in_cons = intf->in_cons;
  in_prod = intf->in_prod;

  smp_rmb();

  /* rest of the code */


The same for the other function.

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

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall]

Hi Bhupinder,

On 06/06/17 18:25, 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 
---
CC: ij
CC: wl
CC: ss
CC: jg
CC: kw

Changes since v3:
- Moved the call to DEFINE_XEN_FLEX_RING from vpl011.h to public/console.h. 
This macro defines
  standard functions to operate on the ring buffer.
- Lock taken while updating the interrupt mask and clear registers in 
mmio_write.
- Use gfn_t instead of xen_pfn_t.
- vgic_free_virq called if there is any error in vpl011 initialization.
- mmio handlers freed if there is any error in vpl011 initialization.
- Removed vpl011->initialized flag usage as the same check could be done
  using vpl011->ring-ref.
- Used return instead of break in the switch handling of emulation of different 
pl011 registers.
- Renamed vpl011_update_spi() to vpl011_update().

Changes since v2:
- Use generic vreg_reg* for read/write of registers emulating pl011.
- Use generic ring buffer functions defined using DEFINE_XEN_FLEX_RING.
- Renamed the SPI injection function to vpl011_update_spi() to reflect level
  triggered nature of pl011 interrupts.
- The pl011 register access address should always be the base address of the
  corresponding register as per section B of the SBSA document. For this reason,
  the register range address access is not allowed.

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

 tools/console/daemon/io.c|   2 +-
 xen/arch/arm/Kconfig |   5 +
 xen/arch/arm/Makefile|   1 +
 xen/arch/arm/vpl011.c| 418 +++
 xen/include/asm-arm/domain.h |   6 +
 xen/include/asm-arm/pl011-uart.h |   2 +
 xen/include/asm-arm/vpl011.h |  74 +++
 xen/include/public/arch-arm.h|   6 +
 xen/include/public/io/console.h  |   4 +


This would require an ACK from Konrad. The addition would also need to 
be justified in the commit message. Although, you probably want to split 
this change in a separate patch.



 9 files changed, 517 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/asm-arm/vpl011.h

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..947f13a 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c


Can you explain why you change the position of the include in io.c?


@@ -21,6 +21,7 @@

 #include "utils.h"
 #include "io.h"
+#include 
 #include 
 #include 
 #include 
@@ -29,7 +30,6 @@

 #include 
 #include 
-#include 
 #include 
 #include 
 #include 



[...]


 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


Please the alphabetical order. Just noticed vtimer is not correctly 
positioned. I will send a patch for that.




 #obj-bin-y += o

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
new file mode 100644
index 000..9b1f27e
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,418 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+
+static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
+{
+return (dabt.size != DABT_DOUBLE_WORD);


Again, please add a comment explaining why we allow all the sizes but 
64-bit.



+}
+
+static void vpl011_update(struct domain *d)
+{
+

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall]

Hi,

On 07/06/17 00:02, Stefano Stabellini wrote:

On Tue, 6 Jun 2017, Bhupinder Thakur wrote:

+static uint8_t vpl011_read_data(struct domain *d)
+{
+unsigned long flags;
+uint8_t data = 0;
+struct vpl011 *vpl011 = >arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+XENCONS_RING_IDX in_cons = intf->in_cons;
+XENCONS_RING_IDX in_prod = intf->in_prod;


After reading the indexes, we always need barriers. In this case:

  smp_rmb();


Well, there are already barrier with the spinlock. However, I am bit 
concern about reading those index without the lock taken. You can have 
concurrent call to vpl011_read_data happening and therefore the indexes 
may have changed when the lock will be taken.






+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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
+{
+data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
+in_cons += 1;
+intf->in_cons = in_cons;
+smp_mb();
+}
+else
+{
+gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
+}
+
+if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
+{
+vpl011->uartfr |= RXFE;
+vpl011->uartris &= ~RXI;
+}
+vpl011->uartfr &= ~RXFF;
+VPL011_UNLOCK(d, flags);


I am pretty sure that the PV console protocol requires us to notify the
other end even on reads. We need to add a notify_via_xen_event_channel
here, I think.


I would agree here. On the previous version, I asked Bhupinder to 
explain why it is necessary and he said: "On second thoughs, 
notification is not required".






+return data;
+}
+
+static void vpl011_write_data(struct domain *d, uint8_t data)
+{
+unsigned long flags;
+struct vpl011 *vpl011 = >arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+XENCONS_RING_IDX out_cons = intf->out_cons;
+XENCONS_RING_IDX out_prod = intf->out_prod;


  smp_mb()


Same remark as above.

[...]


+static void vpl011_data_avail(struct domain *d)
+{
+unsigned long flags;
+struct vpl011 *vpl011 = >arch.vpl011;
+struct xencons_interface *intf = vpl011->ring_buf;
+XENCONS_RING_IDX in_cons = intf->in_cons;
+XENCONS_RING_IDX in_prod = intf->in_prod;
+XENCONS_RING_IDX out_cons = intf->out_cons;
+XENCONS_RING_IDX out_prod = intf->out_prod;
+XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;


  smb_mb()


Ditto.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Stefano Stabellini]

On Tue, 6 Jun 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 
> ---
> CC: ij
> CC: wl
> CC: ss
> CC: jg
> CC: kw
> 
> Changes since v3:
> - Moved the call to DEFINE_XEN_FLEX_RING from vpl011.h to public/console.h. 
> This macro defines
>   standard functions to operate on the ring buffer.
> - Lock taken while updating the interrupt mask and clear registers in 
> mmio_write.
> - Use gfn_t instead of xen_pfn_t.
> - vgic_free_virq called if there is any error in vpl011 initialization.
> - mmio handlers freed if there is any error in vpl011 initialization.
> - Removed vpl011->initialized flag usage as the same check could be done 
>   using vpl011->ring-ref.
> - Used return instead of break in the switch handling of emulation of 
> different pl011 registers.
> - Renamed vpl011_update_spi() to vpl011_update().
> 
> Changes since v2:
> - Use generic vreg_reg* for read/write of registers emulating pl011.
> - Use generic ring buffer functions defined using DEFINE_XEN_FLEX_RING.
> - Renamed the SPI injection function to vpl011_update_spi() to reflect level 
>   triggered nature of pl011 interrupts.
> - The pl011 register access address should always be the base address of the
>   corresponding register as per section B of the SBSA document. For this 
> reason,
>   the register range address access is not allowed.
> 
> Changes since v1:
> - Removed the optimiztion related to sendiing events to xenconsole 
> - Use local variables as ring buffer indices while using the ring buffer
> 
>  tools/console/daemon/io.c|   2 +-
>  xen/arch/arm/Kconfig |   5 +
>  xen/arch/arm/Makefile|   1 +
>  xen/arch/arm/vpl011.c| 418 
> +++
>  xen/include/asm-arm/domain.h |   6 +
>  xen/include/asm-arm/pl011-uart.h |   2 +
>  xen/include/asm-arm/vpl011.h |  74 +++
>  xen/include/public/arch-arm.h|   6 +
>  xen/include/public/io/console.h  |   4 +
>  9 files changed, 517 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/vpl011.c
>  create mode 100644 xen/include/asm-arm/vpl011.h
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e6a886..947f13a 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -21,6 +21,7 @@
>  
>  #include "utils.h"
>  #include "io.h"
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -29,7 +30,6 @@
>  
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> 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..9b1f27e
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
> @@ -0,0 +1,418 @@
> +/*
> + * 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 

[Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[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 
---
CC: ij
CC: wl
CC: ss
CC: jg
CC: kw

Changes since v3:
- Moved the call to DEFINE_XEN_FLEX_RING from vpl011.h to public/console.h. 
This macro defines
  standard functions to operate on the ring buffer.
- Lock taken while updating the interrupt mask and clear registers in 
mmio_write.
- Use gfn_t instead of xen_pfn_t.
- vgic_free_virq called if there is any error in vpl011 initialization.
- mmio handlers freed if there is any error in vpl011 initialization.
- Removed vpl011->initialized flag usage as the same check could be done 
  using vpl011->ring-ref.
- Used return instead of break in the switch handling of emulation of different 
pl011 registers.
- Renamed vpl011_update_spi() to vpl011_update().

Changes since v2:
- Use generic vreg_reg* for read/write of registers emulating pl011.
- Use generic ring buffer functions defined using DEFINE_XEN_FLEX_RING.
- Renamed the SPI injection function to vpl011_update_spi() to reflect level 
  triggered nature of pl011 interrupts.
- The pl011 register access address should always be the base address of the
  corresponding register as per section B of the SBSA document. For this reason,
  the register range address access is not allowed.

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

 tools/console/daemon/io.c|   2 +-
 xen/arch/arm/Kconfig |   5 +
 xen/arch/arm/Makefile|   1 +
 xen/arch/arm/vpl011.c| 418 +++
 xen/include/asm-arm/domain.h |   6 +
 xen/include/asm-arm/pl011-uart.h |   2 +
 xen/include/asm-arm/vpl011.h |  74 +++
 xen/include/public/arch-arm.h|   6 +
 xen/include/public/io/console.h  |   4 +
 9 files changed, 517 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/asm-arm/vpl011.h

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..947f13a 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -21,6 +21,7 @@
 
 #include "utils.h"
 #include "io.h"
+#include 
 #include 
 #include 
 #include 
@@ -29,7 +30,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
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..9b1f27e
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,418 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+
+static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
+{
+return (dabt.size != DABT_DOUBLE_WORD);
+}
+
+static void vpl011_update(struct domain *d)
+{
+struct vpl011 *vpl011 = >arch.vpl011;
+
+/*
+ * TODO: PL011 interrupts 

[Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[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 v3:
- Moved the call to DEFINE_XEN_FLEX_RING from vpl011.h to public/console.h. 
This macro defines
  standard functions to operate on the ring buffer.
- Lock taken while updating the interrupt mask and clear registers in 
mmio_write.
- Use gfn_t instead of xen_pfn_t.
- vgic_free_virq called if there is any error in vpl011 initialization.
- mmio handlers freed if there is any error in vpl011 initialization.
- Removed vpl011->initialized flag usage as the same check could be done 
  using vpl011->ring-ref.
- Used return instead of break in the switch handling of emulation of different 
pl011 registers.
- Renamed vpl011_update_spi() to vpl011_update().

Changes since v2:
- Use generic vreg_reg* for read/write of registers emulating pl011.
- Use generic ring buffer functions defined using DEFINE_XEN_FLEX_RING.
- Renamed the SPI injection function to vpl011_update_spi() to reflect level 
  triggered nature of pl011 interrupts.
- The pl011 register access address should always be the base address of the
  corresponding register as per section B of the SBSA document. For this reason,
  the register range address access is not allowed.

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

 tools/console/daemon/io.c|   2 +-
 xen/arch/arm/Kconfig |   5 +
 xen/arch/arm/Makefile|   1 +
 xen/arch/arm/vpl011.c| 418 +++
 xen/include/asm-arm/domain.h |   6 +
 xen/include/asm-arm/pl011-uart.h |   2 +
 xen/include/asm-arm/vpl011.h |  74 +++
 xen/include/public/arch-arm.h|   6 +
 xen/include/public/io/console.h  |   4 +
 9 files changed, 517 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/vpl011.c
 create mode 100644 xen/include/asm-arm/vpl011.h

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 7e6a886..947f13a 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -21,6 +21,7 @@
 
 #include "utils.h"
 #include "io.h"
+#include 
 #include 
 #include 
 #include 
@@ -29,7 +30,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
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..9b1f27e
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,418 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+
+static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
+{
+return (dabt.size != DABT_DOUBLE_WORD);
+}
+
+static void vpl011_update(struct domain *d)
+{
+struct vpl011 *vpl011 = >arch.vpl011;
+
+/*
+ * TODO: PL011 interrupts are level triggered which means
+