|
[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 Konrad,
On 4 March 2017 at 01:29, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> .snip..
>> + case VPL011_UARTCR_OFFSET:
>> + *r = v->domain->arch.vpl011.control;
>> + break;
>
> Pls add a new newline after each break.
Ok.
>
>> + case VPL011_UARTDR_OFFSET:
>> + vpl011_read_data(v->domain, &ch);
>> + *r = ch;
>> + break;
>> + case VPL011_UARTFR_OFFSET:
>> + *r = v->domain->arch.vpl011.flag;
>> + break;
>> + case VPL011_UARTIMSC_OFFSET:
>> + *r = v->domain->arch.vpl011.intr_mask;
>> + break;
>> + case VPL011_UARTICR_OFFSET:
>> + *r = 0;
>> + break;
>> + case VPL011_UARTRIS_OFFSET:
>> + *r = v->domain->arch.vpl011.raw_intr_status;
>> + break;
>> + case VPL011_UARTMIS_OFFSET:
>> + *r = v->domain->arch.vpl011.raw_intr_status &
>> + v->domain->arch.vpl011.intr_mask;
>> + break;
>> + case VPL011_UARTDMACR_OFFSET:
>> + *r = 0; /* uart DMA is not supported. Here it always returns 0
>> */
>> + break;
>> + case VPL011_UARTRSR_OFFSET:
>> + *r = 0; /* it always returns 0 as there are no physical errors
>> */
>> + break;
>> + default:
>> + printk ("vpl011_mmio_read: invalid switch case %d\n",
>> (int)(info->gpa - GUEST_PL011_BASE));
>
> .. and you still return EMULOK?
>
Now I am returning error for the default case.
> Also is this printk really neccessary?
>
>> + break;
>> + }
>> +
>> + return VPL011_EMUL_OK;
>> +}
>> +
>> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t
>> r, void *priv)
>> +{
>> + unsigned char ch = r;
>> +
>> + switch (info->gpa - GUEST_PL011_BASE)
>> + {
>> + case VPL011_UARTCR_OFFSET:
>> + v->domain->arch.vpl011.control = r;
>> + break;
>> + case VPL011_UARTDR_OFFSET:
>> + vpl011_write_data(v->domain, ch);
>> + break;
>> + case VPL011_UARTIMSC_OFFSET:
>> + v->domain->arch.vpl011.intr_mask = r;
>> + if ( (v->domain->arch.vpl011.raw_intr_status &
>> v->domain->arch.vpl011.intr_mask) )
>> + vgic_vcpu_inject_spi(v->domain,
>> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
>> + 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) )
>> + {
>> + vgic_vcpu_inject_spi(v->domain,
>> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
>> + }
>> + break;
>> + case VPL011_UARTRSR_OFFSET: // nothing to clear
>> + break;
>> + case VPL011_UARTFR_OFFSET: // these are all RO registers
>> + case VPL011_UARTRIS_OFFSET:
>> + case VPL011_UARTMIS_OFFSET:
>> + case VPL011_UARTDMACR_OFFSET:
>> + break;
>> + default:
>> + printk ("vpl011_mmio_write: switch case not handled %d\n",
>> (int)(info->gpa - GUEST_PL011_BASE));
>> + break;
>> + }
>> +
>> + return VPL011_EMUL_OK;
>> +}
>> +
>> +static const struct mmio_handler_ops vpl011_mmio_handler = {
>> + .read = vpl011_mmio_read,
>> + .write = vpl011_mmio_write,
>> +};
>> +
>> +
>
> why the extra newline.
>
>> +
>> +int vpl011_map_guest_page(struct domain *d)
>> +{
>> + int rc=0;
>
> No need for = 0;
Ok.
>> +
>> + /*
>> + * 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);
>> + if ( rc < 0 )
>> + {
>> + printk("Failed to map vpl011 guest PFN\n");
>> + }
>> +
>> + return rc;
>
> Could you just make this whole routine be:
>
> return prepare_ring_for_helper(..)
>
> That printk is not needed I think? I would add it to the caller of this
> function.
>
Ok.
>> +}
>> +
>> +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;
>
> Can you have an macro for this?
>> +
>> + VPL011_LOCK(d, flags);
>
> Please don't. Just use normal spin_lock invocation.
>
>> +
>> + /*`
>> + * check IN ring buffer
>> + */
>
> Please remove the comment. It does not add much context to the code.
>
>> + if ( !VPL011_IN_RING_EMPTY(intf) )
>> + {
>> + /*
>> + * clear the RX FIFO empty flag as the ring is not empty
>> + */
>
> Please remove this comment.
>
>> + d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE);
>> +
>> + /*
>> + * if the buffer is full then set the RX FIFO FULL flag
>
> Also please remove this comment.
>
>> + */
>> + if ( VPL011_IN_RING_FULL(intf) )
>> + d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF);
>> +
>> + /*
>> + * set the RX interrupt status
>> + */
>
> And this one too.
>
> What I would recommend is that you write one nice comment at the start
> of the 'if' saying:
>
> "Have to set RX state regardless whether it is full or has some entries."
>
>> + d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS);
>> + }
>> +
>> + /*
>> + * check OUT ring buffer
>
> Please remove this comment.
>> + */
>> + if ( !VPL011_OUT_RING_FULL(intf) )
>> + {
>> + /*
>> + * if the buffer is not full then clear the TX FIFO full flag
>> + */
>
>
> Please remove this comment.
>> + 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
>
> So does that mean we would send an interrupt even if the in/out rings
> are full?
If the out ring is full then the interrupt would not be raised because
the Tx interrupt status bit would be cleared in UARTRIS register in
vpl011_write_data().
For the in ring, the Rx interrupt would be raised 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]);
>> +
>> + if ( !VPL011_OUT_RING_EMPTY(intf) )
>
> This is asking for a race.
>
> So it may have changed between the first time you read it and this
> one.
>
> Is that OK? Would it still be OK to send an interrupt even if say
> the ring was emtpry at start of the function but became empty now?
>
Yes the out ring state can change between the two times it is checked.
So finally it will be either be empty or non-empty irrespective of
what it was the first time.
If it is non-empty then an event is raised to dom0 to read the data
from the out ring.
> Would it be better if you read all the values from
> the ring at the start of the function in local variables
> and made sure to use 'barrier()' so there are no compiler
> "optimizations" done?
>
>> + {
>> + /*
>> + * raise an interrupt to dom0
>> + */
>
> Please remove this.
>
>> + rc = raw_evtchn_send(d,
>> +
>> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL);
>
> What if HVM_PARAM_VPL011_CONSOLE_EVTCHN is zero? Should you check
> that first?
>
> Or is it given (in which case please add an ASSERT) that the
> HVM_PARAM_VPL011_CONSOLE_EVTCHN is _always_ set?
>
Yes. During vpl011 initialization for this domain, this event channel
will be allocated. I will add an assert.
>> +
>> + if ( rc < 0 )
>> + printk("Failed to send vpl011 interrupt to dom0\n");
>
> gdprintk. But is this useful? How will this help in debugging the
> problem?
>
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +int vpl011_read_data(struct domain *d, unsigned char *data)
>> +{
>> + int rc=0;
>
> No point for this. Just do 'return 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) )
>
> So there is a nice race here. Why don't you just use an 'else' ?
>
Since we are reading the data from the ring buffer in the first step,
the ring buffer may go empty after the first step. If I use else then
I will not enter the 2nd if statement.
>
>> + {
>> + 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)
>> +{
>> + int rc=0;
>
> Pls remove the '=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
>
> Please moreve this comment.
>> + */
>> + 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
>
> Again, why not just an 'else'?
>
>> + */
>> + 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
>> + */
>
> Please remove these comments. They are distracting.
>
> Also a single line comment should be:
>
> /* <comment> */
>
>
>> + 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
>
> Why? Why only the first charcter?
>
I wanted to avoid raising too many events to dom0. Since dom0 is going
to process the data in the ring buffer until it goes empty, it should
be ok to raise an event initially only when the first data is written
to the ring buffer so that dom0 can start reading the data from the
ring buffer.
>> + */
>> + if ( VPL011_RING_DEPTH(intf, out) == 1 )
>> + {
>> + rc = raw_evtchn_send(d,
>> + d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN],
>> NULL);
>
> And again, should we have an assert for HVM_PARAM_VPL011_CONSOLE_EVTCHN or a
> check
> for it?
I will add an assert.
>
>> +
>> + if ( rc < 0 )
>> + printk("Failed to send vpl011 interrupt to dom0\n");
>
> gdprintk.
>
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> + vpl011_data_avail(v->domain);
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d)
>> +{
>> + int rc=0;
>
> You can remove the =0;
>> +
>> + /*
>> + * register xen event channel
>> + */
>
> Please remove this comment.
>
>> + rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id,
>> +
>> vpl011_notification);
>> + if (rc < 0)
>
> Spaces.
>> + {
>> + printk ("Failed to allocate vpl011 event channel\n");
>
> gdprintk
>
> And also not sure if this is needed. Won't the rc be propagated to the
> caller who can print this?
>
>> + 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);
>
> You are not checking vgic_allocate_spi for errors?
>
I have added a check.
>> +
>> + /*
>> + * register mmio handler
>> + */
>
> Please remove these comments. They are just saying what the code
> is doing. Not adding anything extra.
>
>> + register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE,
>> GUEST_PL011_SIZE, NULL);
>
> Please kill that space before (.
>
> And what if register_mmio_handler returns an error?
>
register_mmio_handler() does not return any value.
>> +
>> + /*
>> + * initialize the lock
>> + */
>
> That is a comment that is really not needed.
>
>> + spin_lock_init(&d->arch.vpl011.lock);
>> +
>> + /*
>> + * clear the flag, control and interrupt status registers
>> + */
>
> Why not just do memset on the structure?
>
>> + 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;
>> +
>> + return 0;
>> +}
I will use the memset.
>> 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
>> @@ -0,0 +1,208 @@
>> +/*
>> + * include/xen/vpl011.h
>> + *
>> + * 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 <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef _VPL011_H_
>> +
>> +#define _VPL011_H_
>> +
>> +/*
>> + * register offsets
>> + */
>> +#define VPL011_UARTDR_OFFSET 0x0 // data register (RW)
>
> Wrong type of comments. Pls use /* */ variant
Ok.
>
>> +#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
>> +
>> +/*
>> + * control register bits - RW
>> + */
>> +#define VPL011_UARTCR_UARTEN_BIT 0
>> +#define VPL011_UARTCR_UARTEN (1<<VPL011_UARTCR_UARTEN_BIT)
>> +#define VPL011_UARTCR_TXE_BIT 8
>> +#define VPL011_UARTCR_TXE (1<<VPL011_UARTCR_TXE_BIT)
>> +#define VPL011_UARTCR_RXE_BIT 9
>> +#define VPL011_UARTCR_RXE (1<<VPL011_UARTCR_RXE_BIT)
>> +
>> +/*
>> + * Flag register bits - RO
>> + */
>> +#define VPL011_UARTFR_CTS_BIT 0 // clear to send
>> +#define VPL011_UARTFR_CTS (1<<VPL011_UARTFR_CTS_BIT)
>> +#define VPL011_UARTFR_DSR_BIT 1 // data set ready
>> +#define VPL011_UARTFR_DSR (1<<VPL011_UARTFR_DSR_BIT)
>> +#define VPL011_UARTFR_DCD_BIT 2 // data carrier detect
>> +#define VPL011_UARTFR_DCD (1<<VPL011_UARTFR_DCD_BIT)
>> +#define VPL011_UARTFR_BUSY_BIT 3 // uart busy
>> +#define VPL011_UARTFR_BUSY (1<<VPL011_UARTFR_BUSY_BIT)
>> +#define VPL011_UARTFR_RXFE_BIT 4 // receive fifo empty
>> +#define VPL011_UARTFR_RXFE (1<<VPL011_UARTFR_RXFE_BIT)
>> +#define VPL011_UARTFR_TXFF_BIT 5 // transmit fifo full
>> +#define VPL011_UARTFR_TXFF (1<<VPL011_UARTFR_TXFF_BIT)
>> +#define VPL011_UARTFR_RXFF_BIT 6 // receive fifo full
>> +#define VPL011_UARTFR_RXFF (1<<VPL011_UARTFR_RXFF_BIT)
>> +#define VPL011_UARTFR_TXFE_BIT 7 // transmit fifo empty
>> +#define VPL011_UARTFR_TXFE (1<<VPL011_UARTFR_TXFE_BIT)
>> +#define VPL011_UARTFR_RI_BIT 8 // ring indicator
>> +#define VPL011_UARTFR_RI (1<<VPL011_UARTFR_RI_BIT)
>> +
>> +/*
>> + * UART raw interrupt status bits - RO
>> + */
>> +#define VPL011_UARTRIS_RIRMIS_BIT 0
>> +#define VPL011_UARTRIS_RIRMIS (1<<VPL011_UARTRIS_RIRMIS_BIT)
>> +#define VPL011_UARTRIS_CTSRMIS_BIT 1
>> +#define VPL011_UARTRIS_CTSRMIS (1<<VPL011_UARTRIS_CTSRMIS_BIT)
>> +#define VPL011_UARTRIS_DCDRMIS_BIT 2
>> +#define VPL011_UARTRIS_DCDRMIS (1<<VPL011_UARTRIS_DCDRMIS_BIT)
>> +#define VPL011_UARTRIS_DSRRMIS_BIT 3
>> +#define VPL011_UARTRIS_DSRRMIS (1<<VPL011_UARTRIS_DSRRMIS_BIT)
>> +#define VPL011_UARTRIS_RXRIS_BIT 4
>> +#define VPL011_UARTRIS_RXRIS (1<<VPL011_UARTRIS_RXRIS_BIT)
>> +#define VPL011_UARTRIS_TXRIS_BIT 5
>> +#define VPL011_UARTRIS_TXRIS (1<<VPL011_UARTRIS_TXRIS_BIT)
>> +#define VPL011_UARTRIS_RTRIS_BIT 6
>> +#define VPL011_UARTRIS_RTRIS (1<<VPL011_UARTRIS_RTRIS_BIT)
>> +#define VPL011_UARTRIS_FERIS_BIT 7
>> +#define VPL011_UARTRIS_FERIS (1<<VPL011_UARTRIS_FERIS_BIT)
>> +#define VPL011_UARTRIS_PERIS_BIT 8
>> +#define VPL011_UARTRIS_PERIS (1<<VPL011_UARTRIS_PERIS_BIT)
>> +#define VPL011_UARTRIS_BERIS_BIT 9
>> +#define VPL011_UARTRIS_BERIS (1<<VPL011_UARTRIS_BERIS_BIT)
>> +#define VPL011_UARTRIS_OERIS_BIT 10
>> +#define VPL011_UARTRIS_OERIS (1<<VPL011_UARTRIS_OERIS_BIT)
>> +
>> +/*
>> + * UART masked interrupt status bits - RO
>> + */
>> +#define VPL011_UARTMIS_RIMMIS_BIT 0
>> +#define VPL011_UARTMIS_RIMMIS (1<<VPL011_UARTMIS_RIMMIS_BIT)
>> +#define VPL011_UARTMIS_CTSMMIS_BIT 1
>> +#define VPL011_UARTMIS_CTSMMIS (1<<VPL011_UARTMIS_CTSMMIS_BIT)
>> +#define VPL011_UARTMIS_DCDMMIS_BIT 2
>> +#define VPL011_UARTMIS_DCDMMIS (1<<VPL011_UARTMIS_DCDMMIS_BIT)
>> +#define VPL011_UARTMIS_DSRMMIS_BIT 3
>> +#define VPL011_UARTMIS_DSRMMIS (1<<VPL011_UARTMIS_DSRMMIS_BIT)
>> +#define VPL011_UARTMIS_RXMIS_BIT 4
>> +#define VPL011_UARTMIS_RXMIS (1<<VPL011_UARTMIS_RXMIS_BIT)
>> +#define VPL011_UARTMIS_TXMIS_BIT 5
>> +#define VPL011_UARTMIS_TXMIS (1<<VPL011_UARTMIS_TXMIS_BIT)
>> +#define VPL011_UARTMIS_RTMIS_BIT 6
>> +#define VPL011_UARTMIS_RTMIS (1<<VPL011_UARTMIS_RTMIS_BIT)
>> +#define VPL011_UARTMIS_FEMIS_BIT 7
>> +#define VPL011_UARTMIS_FEMIS (1<<VPL011_UARTMIS_FEMIS_BIT)
>> +#define VPL011_UARTMIS_PEMIS_BIT 8
>> +#define VPL011_UARTMIS_PEMIS (1<<VPL011_UARTMIS_PEMIS_BIT)
>> +#define VPL011_UARTMIS_BEMIS_BIT 9
>> +#define VPL011_UARTMIS_BEMIS (1<<VPL011_UARTMIS_BEMIS_BIT)
>> +#define VPL011_UARTMIS_OEMIS_BIT 10
>> +#define VPL011_UARTMIS_OEMIS (1<<VPL011_UARTMIS_OEMIS_BIT)
>> +
>> +/*
>> + * UART interrupt clear bits - WO
>> + */
>> +#define VPL011_UARTICR_RIMIC_BIT 0
>> +#define VPL011_UARTICR_RIMIC (1<<VPL011_UARTICR_RIMIC_BIT)
>> +#define VPL011_UARTICR_CTSMIC_BIT 1
>> +#define VPL011_UARTICR_CTSMIC (1<<VPL011_UARTICR_CTSMIC_BIT)
>> +#define VPL011_UARTICR_DCDMIC_BIT 2
>> +#define VPL011_UARTICR_DCDMIC (1<<VPL011_UARTICR_DCDMIC_BIT)
>> +#define VPL011_UARTICR_DSRMIC_BIT 3
>> +#define VPL011_UARTICR_DSRMIC (1<<VPL011_UARTICR_DSRMIC_BIT)
>> +#define VPL011_UARTICR_RXIC_BIT 4
>> +#define VPL011_UARTICR_RXIC (1<<VPL011_UARTICR_RXIC_BIT)
>> +#define VPL011_UARTICR_TXIC_BIT 5
>> +#define VPL011_UARTICR_TXIC (1<<VPL011_UARTICR_TXIC_BIT)
>> +#define VPL011_UARTICR_RTIC_BIT 6
>> +#define VPL011_UARTICR_RTIC (1<<VPL011_UARTICR_RTIC_BIT)
>> +#define VPL011_UARTICR_FEIC_BIT 7
>> +#define VPL011_UARTICR_FEIC (1<<VPL011_UARTICR_FEIC_BIT)
>> +#define VPL011_UARTICR_PEIC_BIT 8
>> +#define VPL011_UARTICR_PEIC (1<<VPL011_UARTICR_PEIC_BIT)
>> +#define VPL011_UARTICR_BEIC_BIT 9
>> +#define VPL011_UARTICR_BEIC (1<<VPL011_UARTICR_BEIC_BIT)
>> +#define VPL011_UARTICR_OEIC_BIT 10
>> +#define VPL011_UARTICR_OEIC (1<<VPL011_UARTICR_OEIC_BIT)
>> +
>> +/*
>> + * UART interrupt mask set/clear bits - RW
>> + */
>> +#define VPL011_UARTIMSC_RIMIM_BIT 0
>> +#define VPL011_UARTIMSC_RIMIM (1<<VPL011_UARTIMSC_RIMIM_BIT)
>> +#define VPL011_UARTIMSC_CTSMIM_BIT 1
>> +#define VPL011_UARTIMSC_CTSMIM (1<<VPL011_UARTIMSC_CTSMIM_BIT)
>> +#define VPL011_UARTIMSC_DCDMIM_BIT 2
>> +#define VPL011_UARTIMSC_DCDMIM (1<<VPL011_UARTIMSC_DCDMIM_BIT)
>> +#define VPL011_UARTIMSC_DSRMIM_BIT 3
>> +#define VPL011_UARTIMSC_DSRMIM (1<<VPL011_UARTIMSC_DSRMIM_BIT)
>> +#define VPL011_UARTIMSC_RXIM_BIT 4
>> +#define VPL011_UARTIMSC_RXIM (1<<VPL011_UARTIMSC_RXIM_BIT)
>> +#define VPL011_UARTIMSC_TXIM_BIT 5
>> +#define VPL011_UARTIMSC_TXIM (1<<VPL011_UARTIMSC_TXIM_BIT)
>> +#define VPL011_UARTIMSC_RTIM_BIT 6
>> +#define VPL011_UARTIMSC_RTIM (1<<VPL011_UARTIMSC_RTIM_BIT)
>> +#define VPL011_UARTIMSC_FEIM_BIT 7
>> +#define VPL011_UARTIMSC_FEIM (1<<VPL011_UARTIMSC_FEIM_BIT)
>> +#define VPL011_UARTIMSC_PEIM_BIT 8
>> +#define VPL011_UARTIMSC_PEIM (1<<VPL011_UARTIMSC_PEIM_BIT)
>> +#define VPL011_UARTIMSC_BEIM_BIT 9
>> +#define VPL011_UARTIMSC_BEIM (1<<VPL011_UARTIMSC_BEIM_BIT)
>> +#define VPL011_UARTIMSC_OEIM_BIT 10
>> +#define VPL011_UARTIMSC_OEIM (1<<VPL011_UARTIMSC_OEIM_BIT)
>> +
>> +
>> +/*
>> + * helper macros
>> + */
>> +#define VPL011_RING_DEPTH(intf,dir) (((intf)->dir ## _prod - (intf)->dir ##
>> _cons))
>> +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir)-1)
>> +
>> +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0)
>> +
>> +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0)
>> +
>> +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) ==
>> VPL011_RING_MAX_DEPTH(intf, in))
>> +
>> +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) ==
>> VPL011_RING_MAX_DEPTH(intf,out))
>> +
>> +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock,
>> flags)
>> +#define VPL011_UNLOCK(d,flags)
>> spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
>> +
>> +/*
>> + * MMIO return values
>> + */
>> +#define VPL011_EMUL_OK 1
>> +#define VPL011_EMUL_FAIL 0
>> +
>> +int domain_vpl011_init(struct domain *d);
>> +int vpl011_map_guest_page(struct domain *d);
>> +int vpl011_read_data(struct domain *d, unsigned char *data);
>> +int vpl011_write_data(struct domain *d, unsigned char data);
>> +
>> +#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
>> 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
>
> I thought by default it would be 'n'?
>
> Or perhaps defauly y if CONFIG_ARM or such?
I moved this config option to xen/arch/arm/Kconfig.
>
>> + ---help---
>> + Allows a guest to use pl011 UART as a console
>> endmenu
>> 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;
>> };
>>
>> +
>
> ??
>> 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
>> } __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)
>>
>> +
>
> ???
I will remove unnecessary newlines.
>> #define GUEST_RAM_BANKS 2
>>
>> #define GUEST_RAM0_BASE xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB
>> */
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel
Regards,
Bhupinder
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |