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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Bhupinder,

On 23/03/17 09:44, Bhupinder Thakur wrote:
> > > > > +            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?
> I think OE bit is more applicable to physical UART, where data may
> arrive while the hardware RX FIFO is already full.
From my understanding, the ring is the RX FIFO and xenconsole will act 
of the "user" of the hardware. Hence why I though OE should be set if 
the ring is full.
Anyway, both point of views are valid, you should just document what has 
been chosen.
> > > > > +            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.
> I will use true/false.
It is not what I said. We should not mix true/false with int. My point 
was that I have an action to switch the return type for int to bool. For 
the time being we should use 0/1.
Cheers,

--
Julien Grall

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