|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 01/11] xen/arm: vpl011: Add pl011 uart emulation in Xen
.Hi Julien,
On 5 March 2017 at 17:42, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Bhupinder,
>
> On 21/02/17 11:25, Bhupinder Thakur wrote:
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> new file mode 100644
>> index 0000000..88ba968
>> --- /dev/null
>> +++ b/xen/arch/arm/vpl011.c
>
>
> [...]
>
>> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info,
>> register_t r, void *priv)
>
>
> See my comment on the vpl011_mmio_read about the structure of the function.
>
>> +{
>> + unsigned char ch = r;
>> +
>> + switch (info->gpa - GUEST_PL011_BASE)
>> + {
>> + case VPL011_UARTCR_OFFSET:
>
>
> This register is not required in the SBSA UART.
Yes, I checked that. I will remove it.
>
>> + v->domain->arch.vpl011.control = r;
>> + break;
>> + case VPL011_UARTDR_OFFSET:
>> + vpl011_write_data(v->domain, ch);
>
>
> The implicit casting of register_t to ch is a bit confusing. Also I would
> prefer to use uint8_t as it reflects the size of the field.
I have removed implicit casing with explicit casting.
>
> Lastly, what if vpl011_write_data is returning an error?
>
Normally vpl011_write_data() should not fail since the guest should
stop writing more data once the ring buffer goes full and TXFF bit is
set in UARTFR.
So there should always be space in the ring buffer for the next data
when a mmio write happens.
If the guest still writes when ring buffer is already full then data
would be lost.
>> + break;
>> + case VPL011_UARTIMSC_OFFSET:
>> + v->domain->arch.vpl011.intr_mask = r;
>
>
> You need to sanitize the value written and make sure reserved field and
> Write As Ones register and handle correctly (see the spec).
>
>> + if ( (v->domain->arch.vpl011.raw_intr_status &
>> v->domain->arch.vpl011.intr_mask) )
>
>
> This line and the line below are over 80 columns. Also the code in general
> would benefits if you define a local variable to point to
> v->domain->arch.vpl011.
I will use a local variable for v->domain->arch.vpl011
>
>> + vgic_vcpu_inject_spi(v->domain,
>> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
>
>
> I don't think we need an HVM_PARAM for the pl011 SPI. We cannot hardcode
> this in the guest layout (see include/public/arch-arm.h).
Please see my comment below on how to reserve 1 SPI for vpl011.
>
>> + break;
>> + case VPL011_UARTICR_OFFSET:
>> + /*
>> + * clear all bits which are set in the input
>> + */
>> + v->domain->arch.vpl011.raw_intr_status &= ~r;
>> + if ( (v->domain->arch.vpl011.raw_intr_status &
>> v->domain->arch.vpl011.intr_mask) )
>> + {
>
>
> { } are not necessary.
>
>> + vgic_vcpu_inject_spi(v->domain,
>> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
>> + }
>
>
> The if is exactly the same as the on in IMSC. This is the call of an helper
> to check the interrupt state and inject one if necessary.
>
>> + break;
>> + case VPL011_UARTRSR_OFFSET: // nothing to clear
>
>
> The coding style for single line comment in Xen is /* ... */
>
> Also, I think we may want to handle Overrun error as the ring may be full.
>
>> + break;
>> + case VPL011_UARTFR_OFFSET: // these are all RO registers
>> + case VPL011_UARTRIS_OFFSET:
>> + case VPL011_UARTMIS_OFFSET:
>> + case VPL011_UARTDMACR_OFFSET:
>
>
> Please have a look at the vGICv2/vGICv3 emulation see how we implemented
> write ignore register.
>
Ok.
>> + break;
>> + default:
>> + printk ("vpl011_mmio_write: switch case not handled %d\n",
>> (int)(info->gpa - GUEST_PL011_BASE));
>
>
> See my comment about the prinkt in the read emulation.
>
>> + break;
>> + }
>> +
>> + return VPL011_EMUL_OK;
>> +}
>> +
>> +static const struct mmio_handler_ops vpl011_mmio_handler = {
>> + .read = vpl011_mmio_read,
>> + .write = vpl011_mmio_write,
>> +};
>> +
>> +
>> +
>
>
> Only newline is sufficient. This was mention by Konrad and I will try to
> avoid repeating his comments.
>
>> +int vpl011_map_guest_page(struct domain *d)
>> +{
>> + int rc=0;
>> +
>> + /*
>> + * map the guest PFN to Xen address space
>> + */
>> + rc = prepare_ring_for_helper(d,
>> +
>> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN],
>> + &d->arch.vpl011.ring_page,
>> + (void **)&d->arch.vpl011.ring_buf);
>
>
> Why do you need the cast?
>
I will remove the casting.
> Also I cannot find any code in this series which destroy the ring. Please
> have a helper called vpl011_unmap_guest_page to do this job and call when
> the domain is destroyed.
>
Ok.
>
>> + if ( rc < 0 )
>> + {
>> + printk("Failed to map vpl011 guest PFN\n");
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +static int vpl011_data_avail(struct domain *d)
>> +{
>> + int rc=0;
>> + unsigned long flags;
>> +
>> + struct console_interface *intf=(struct console_interface
>> *)d->arch.vpl011.ring_buf;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + /*`
>> + * check IN ring buffer
>> + */
>> + if ( !VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + /*
>> + * clear the RX FIFO empty flag as the ring is not empty
>> + */
>> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE);
>> +
>> + /*
>> + * if the buffer is full then set the RX FIFO FULL flag
>> + */
>> + if ( VPL011_IN_RING_FULL(intf) )
>> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF);
>> +
>> + /*
>> + * set the RX interrupt status
>> + */
>> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS);
>> + }
>> +
>> + /*
>> + * check OUT ring buffer
>> + */
>> + if ( !VPL011_OUT_RING_FULL(intf) )
>> + {
>> + /*
>> + * if the buffer is not full then clear the TX FIFO full flag
>> + */
>> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF);
>> +
>> + /*
>> + * set the TX interrupt status
>> + */
>> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS);
>> +
>> + if ( VPL011_OUT_RING_EMPTY(intf) )
>> + {
>> + /*
>> + * clear the uart busy flag and set the TX FIFO empty flag
>> + */
>> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY);
>> + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE);
>> + }
>> + }
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + /*
>> + * send an interrupt if it is not masked
>> + */
>> + if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) )
>> + vgic_vcpu_inject_spi(d,
>> (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
>
>
> See my comment above about having an helper for this code.
Ok. I will add a helper function.
>
>> +
>> + if ( !VPL011_OUT_RING_EMPTY(intf) )
>> + {
>> + /*
>> + * raise an interrupt to dom0
>
>
> The backend console does not necessarily live in DOM0. Anyway, Konrad
> requested to drop the comment and I am happy with that.
>
>> + */
>> + rc = raw_evtchn_send(d,
>> +
>> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL);
>
>
> You are using a function that is been defined in a later patch (i.w #3). All
> the patch should be able to compile without applying upcoming patch to allow
> bisection.
>
> Ideally, Xen should still be functional for every patch. But it is a best
> effort.
>
I will reorder the patches accordingly.
>> +
>> + if ( rc < 0 )
>> + printk("Failed to send vpl011 interrupt to dom0\n");
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +int vpl011_read_data(struct domain *d, unsigned char *data)
>
>
> This function does not seem to be used outside of this file. So why do you
> export it?
>
Made it a static function.
>
>> +{
>> + int rc=0;
>> + unsigned long flags;
>> + struct console_interface *intf=(struct console_interface
>> *)d->arch.vpl011.ring_buf;
>> +
>> + *data = 0;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + /*
>> + * if there is data in the ring buffer then copy it to the output
>> buffer
>> + */
>> + if ( !VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)];
>> + }
>> +
>> + /*
>> + * if the ring buffer is empty then set the RX FIFO empty flag
>> + */
>> + if ( VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE);
>> + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS);
>> + }
>> +
>> + /*
>> + * clear the RX FIFO full flag
>> + */
>> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF);
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + return rc;
>> +}
>> +
>> +int vpl011_write_data(struct domain *d, unsigned char data)
>
>
> Same has for vpl011_read_data.
>
>
>> +{
>> + int rc=0;
>> + unsigned long flags;
>> + struct console_interface *intf=(struct console_interface
>> *)d->arch.vpl011.ring_buf;
>> +
>> + VPL011_LOCK(d, flags);
>> +
>> + /*
>> + * if there is space in the ring buffer then write the data
>> + */
>> + if ( !VPL011_OUT_RING_FULL(intf) )
>> + {
>> + intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] =
>> data;
>> + smp_wmb();
>> + }
>> +
>> + /*
>> + * if there is no space in the ring buffer then set the
>> + * TX FIFO FULL flag
>> + */
>> + if ( VPL011_OUT_RING_FULL(intf) )
>> + {
>> + d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF);
>> + d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS);
>> + }
>> +
>> + /*
>> + * set the uart busy status
>> + */
>> + d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY);
>> +
>> + /*
>> + * clear the TX FIFO empty flag
>> + */
>> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE);
>> +
>> + VPL011_UNLOCK(d, flags);
>> +
>> + /*
>> + * raise an event to dom0 only if it is the first character in the
>> buffer
>> + */
>> + if ( VPL011_RING_DEPTH(intf, out) == 1 )
>> + {
>> + rc = raw_evtchn_send(d,
>> +
>> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL);
>> +
>> + if ( rc < 0 )
>> + printk("Failed to send vpl011 interrupt to dom0\n");
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> + vpl011_data_avail(v->domain);
>
>
> vpl011_data_avail is returning an error but you don't check. What is the
> point of it then?
>
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d)
>
>
> I was expected a destroy function to clean-up the vpl011 emulation.
>
I will add a deinit function.
>> +{
>> + int rc=0;
>> +
>> + /*
>> + * register xen event channel
>> + */
>> + rc = alloc_unbound_xen_event_channel(d, 0,
>> current->domain->domain_id,
>> +
>> vpl011_notification);
>
>
> This line looks wrong to me. The console backend may not live in the same
> domain as the toolstack.
How should I figure out the correct domain where xenconsoled is
running?
>
>> + if (rc < 0)
>> + {
>> + printk ("Failed to allocate vpl011 event channel\n");
>> + return rc;
>> + }
>> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc;
>> +
>> + /*
>> + * allocate an SPI VIRQ for the guest
>> + */
>> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] =
>> vgic_allocate_spi(d);
>
>
> It makes little sense to hardcode the MMIO region and not the SPI. Also, I
> would rather avoid to introduce new HVM_PARAM when not necessary. So
> hardcoding the value looks more sensible to me.
>
So for reserving a SPI for vpl011, should I reserve one bit in the
vgic.allocated_irqs bitmap in domain_vgic_init(), say 988, for vpl011?
I think if I am going to reserve 1 SPI for vpl011 then I need not bump
up nr_spis by 1.
>> +
>> + /*
>> + * register mmio handler
>> + */
>> + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE,
>> GUEST_PL011_SIZE, NULL);
>
>
> Coding style. No space between the function name and (.
>
Ok. I will remove the space.
>> +
>> + /*
>> + * initialize the lock
>> + */
>> + spin_lock_init(&d->arch.vpl011.lock);
>> +
>> + /*
>> + * clear the flag, control and interrupt status registers
>> + */
>> + d->arch.vpl011.control = 0;
>> + d->arch.vpl011.flag = 0;
>> + d->arch.vpl011.intr_mask = 0;
>> + d->arch.vpl011.intr_clear = 0;
>> + d->arch.vpl011.raw_intr_status = 0;
>> + d->arch.vpl011.masked_intr_status = 0;
>
>
>
> The domain structure is already zeroed. So no need to 0 it again.
>
Ok.
>> +
>> + return 0;
>> +}
>
>
> Missing e-macs magic here.
>
>> diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h
>> new file mode 100644
>> index 0000000..f2c577f
>> --- /dev/null
>> +++ b/xen/arch/arm/vpl011.h
>
>
> Header should live in include.
>
I will move vpl011.h to xen/include/xen.
> [...]
>
>> +#ifndef _VPL011_H_
>> +
>> +#define _VPL011_H_
>> +
>> +/*
>> + * register offsets
>> + */
>
>
> We already define the PL011 register in include/asm-arm/pl011-uart.h. Please
> re-use them rather than re-inventing the wheel.
>
>> +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW)
>> +#define VPL011_UARTRSR_OFFSET 0x4 // receive status and error clear
>> register (RW)
>> +#define VPL011_UARTFR_OFFSET 0x18 // flag register (RO)
>> +#define VPL011_UARTRIS_OFFSET 0x3c // raw interrupt status register
>> (RO)
>> +#define VPL011_UARTMIS_OFFSET 0x40 // masked interrupt status register
>> (RO)
>> +#define VPL011_UARTIMSC_OFFSET 0x38 // interrupt mask set/clear register
>> (RW)
>> +#define VPL011_UARTICR_OFFSET 0x44 // interrupt clear register (WO)
>> +#define VPL011_UARTCR_OFFSET 0x30 // uart control register
>> +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register
>
>
I will reuse the pl011-uart definitions.
>
> [...]
>
>> +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1))
>> +struct console_interface {
>> + char in[1024];
>> + char out[2048];
>> + uint32_t in_cons, in_prod;
>> + uint32_t out_cons, out_prod;
>> +};
>> +#endif
>
>
> The communication between Xen and the console backend is based on the PV
> console ring. It would be easier to re-use it rather than recreating one.
>
I will reuse the ring definition.
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index f2ecbc4..7e2feac 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP
>> The only user of this is Live patching.
>>
>> If unsure, say Y.
>> +
>> +config VPL011_CONSOLE
>> + bool "Emulated pl011 console support"
>> + default y
>> + ---help---
>> + Allows a guest to use pl011 UART as a console
>> endmenu
>
>
> This should go in arch/arm/Kconfig
>
Ok.
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 2d6fbb1..ff2403a 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -40,6 +40,7 @@ struct vtimer {
>> uint64_t cval;
>> };
>>
>> +
>
>
> Spurious line.
>
>> struct arch_domain
>> {
>> #ifdef CONFIG_ARM_64
>> @@ -131,6 +132,20 @@ struct arch_domain
>> struct {
>> uint8_t privileged_call_enabled : 1;
>> } monitor;
>> +
>> +#ifdef CONFIG_VPL011_CONSOLE
>> + struct vpl011 {
>> + void *ring_buf;
>> + struct page_info *ring_page;
>> + uint32_t flag; /* flag register */
>> + uint32_t control; /* control register */
>> + uint32_t intr_mask; /* interrupt mask register*/
>> + uint32_t intr_clear; /* interrupt clear register */
>> + uint32_t raw_intr_status; /* raw interrupt status register
>> */
>> + uint32_t masked_intr_status; /* masked interrupt register */
>> + spinlock_t lock;
>> + } vpl011;
>> +#endif
>
>
> I think the structure should be defined in vpl011.h.
>
I will move the definition to vpl011.h.
>> } __cacheline_aligned;
>>
>> struct arch_vcpu
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index bd974fb..1d4548f 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t;
>> #define GUEST_ACPI_BASE 0x20000000ULL
>> #define GUEST_ACPI_SIZE 0x02000000ULL
>>
>> +/* PL011 mappings */
>> +#define GUEST_PL011_BASE 0x22000000ULL
>> +#define GUEST_PL011_SIZE 0x00001000ULL
>> +
>> /*
>> * 16MB == 4096 pages reserved for guest to use as a region to map its
>> * grant table in.
>> @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t;
>> #define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000)
>> #define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000)
>>
>> +
>
>
> Spurious line.
>
>> #define GUEST_RAM_BANKS 2
>>
>> #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @
>> 1GB */
>>
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |