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

[Julien Grall <julien.grall@xxxxxxx>]

On 23/03/17 09:14, Bhupinder Thakur wrote:
> .Hi Julien,
Hi Bhupinder,

> 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
> > 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.
If the return code is meaningless, then the function should be void. 
Also, the behavior should be documented.
[...]

> > > +{
> > > +    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?
I think you would to pass the console domain id from the toolstack (see 
console->backend_domid).
> > > +    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.
Regardless the solution solution, nr_spis still need to be incremented. 
This field is used to know the number of SPIs exposed to the guest.
Cheers,

--
Julien Grall

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