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

[Julien Grall <julien.grall@xxxxxxx>]

On 03/21/2017 01:27 PM, Bhupinder Thakur wrote:
> Hi Julien,
Hi Bhupinder,

> On 26 February 2017 at 22:37, Julien Grall <julien.grall@xxxxxxx> wrote:
> > > +            break;
> > > +        case VPL011_UARTDR_OFFSET:
> > > +            vpl011_read_data(v->domain, &ch);
> >
> > Should not you check the return value of vpl011_read_data? Also, what if
> > there is no data?
> This condition should not happen because the RX FIFO empty bit would
> be set in the UARTFR register when the last data is read from the ring
> buffer and the the guest is not supposed to issue next read until the
> RX FIFO empty bit is cleared indicating there is more data now.
What you describe is how a well-behave guest will interact with the 
pl011 emulation. It does not describe what would happen if a misbehaved 
guest will read continuously the register.
I cannot find any things in the spec about what should be the state of 
the register if no data is available. So I guess we just need to ensure 
that we don't leak in information from the stack. This seems to be 
addressed by *data = 0 in "vpl011_read_data".
Please document it to avoid removing it by mistake in the future.

> > > +            *r = ch;
> > > +            break;
> > > +        case VPL011_UARTFR_OFFSET:
> > > +            *r = v->domain->arch.vpl011.flag;
> >
> > I am fairly surprised that none of this code is actually protected by lock.
> > For instance the update of flag is not atomic. So is it safe?
> For reading, I thought no locking was required, as I believe a 32-bit
> value read/write on ARM should be atomic. So if the value is modified
> in some other context while it is being read in another context, the
> reader should see either the old value or the new value.
> For register writes, yes I need to take a lock where I am updating
> certain bits in the register. I will add the locking there.
Fair point. I was more worry about ordering between read/write between 
multiple vCPU. But I guess we don't care if the read does not return an 
update to date value.
> > > +            break;
> > > +        case VPL011_UARTIMSC_OFFSET:
> > > +            *r = v->domain->arch.vpl011.intr_mask;
> > > +            break;
> > > +        case VPL011_UARTICR_OFFSET:
> > > +            *r = 0;
> >
> > Looking at the spec, this register is write-only. So why do you implement
> > RAZ?
> In such cases, where the guest tries to write to RO register or tries
> to read a WO register, should I send a abort to the guest?
I don't see any behavior requirement in the spec. So I would send an 
abort to the guest (e.g return 0 in the function).
> > > +            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 */
> >
> > My understanding of the spec is DMA is not optional. So what would happen if
> > the guest tries to enable it?
> >
> > > +            break;
> > > +        case VPL011_UARTRSR_OFFSET:
> > > +            *r = 0; /* it always returns 0 as there are no physical
> > > errors */
> >
> > This register contains contains the bit OE that tells whether the FIFO is
> > full or not. The FIFO here is the PV ring, so maybe we should set this bit
> > if the ring is full.
> The OE condition will not happen in this case since xenconsole will
> not write more data to the guest if the ring buffer is full. There is
> a separate UARTFR status bit which indicates whether the ring buffer
> is full.
Sorry I still don't get it. Xenconsole will likely have to hold 
characters that are not written in the PV console. I would have expected 
that this would be used to set the OE bit. Did I miss anything?
> > > +            break;
> > > +        default:
> > > +            printk ("vpl011_mmio_read: invalid switch case %d\n",
> > > (int)(info->gpa - GUEST_PL011_BASE));
> >
> > Coding style: printk(...).
> >
> > Also, printk is not ratelimited by default. Please use gprintk(...) which
> > will be ratelimited and print the domain information. This is useful when
> > you have multiple guest.
> Replaced printk with gprintk.
>
> > > +            break;
> > > +    }
> > > +
> > > +    return VPL011_EMUL_OK;
> >
> > Please use plain value as the return is not pl011 specific. Also, I am a bit
> > surprised that you return "ok" even when a register as not been emulated.
> > IHMO, a data abort should be sent to the guest.
> Corrected this. Now it returns an error incase the register is not
> emulated . For the return values, I see that typically values 1/0 are
> returned like in vgic-v2/3.c. Are there some common macros which I can
> use?
No. I want to replace 0/1 by false/true but I never had the time to do 
the work.
Cheers,

--
Julien Grall

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