Re: [Xen-ia64-devel] IOSAPIC virtualisation

[Tristan Gingold <Tristan.Gingold@xxxxxxxx>]

Le Jeudi 02 FÃvrier 2006 19:16, Alex Williamson a Ãcrit :
> Hi Tristan,
>
>    I took another quick skim through and have a few more comments.  It's
> looking good, thanks for all your work here!
>
>       Alex
>
> On Thu, 2006-02-02 at 15:01 +0100, Tristan Gingold wrote:
> > -       *pval = ia64_getreg(_IA64_REG_CR_LID);
> > +       *pval = vcpu->vcpu_id * 0x01010000;
>
>    A comment describing this magic multiplier might be nice.
Yes.
Currently I put vcpu_id in eid and id.
I will document this for SMP dom0/domU.

> > +#ifdef CONFIG_XEN
> > +       xen_iosapic_write (rte, IOSAPIC_RTE_HIGH (rte_index), high32);
> > +       xen_iosapic_write (rte, IOSAPIC_RTE_LOW (rte_index), low32);
> > +#else
> > +       iosapic_write(addr, IOSAPIC_RTE_HIGH(rte_index), high32);
> > +       iosapic_write(addr, IOSAPIC_RTE_LOW(rte_index), low32);
> > +#endif
>
>    Should we make these runtime checks (ie. if (running_on_xen))?  We
> may end up with other things that prevent transparent
> paravirtualization, but this probably shouldn't.
Transparent paravirtualization is kept in running_on_xen, which does the check
on running_on_xen.

>    Another random idea; it might also be cleaner for eventual upstream
> inclusion if the xen code were pushed up into the iosapic_read/write/eoi
> functions.  Maybe modify the functions to take an rte instead of the
> addr.  Then the running_on_xen or CONFIG_XEN checks could be fairly
> consolidated.
I agree on this.  That's the reason why I made the check in 
xen_iosapic_write/read.  Maybe the functions should be renamed later ?

> > +       else {
> > +               /* Interrupt is unmasked.  */
> > +               if (intr->low32 & IOSAPIC_MASK) {
> > +                       /* And was masked.  */
> > +                       rte->vcpu = current;
> > +                       rte->vcpu_vec = val & 0xff;
> > +                       spin_unlock_irqrestore(&iosapic_lock, flags);
> > +                       unmask_vec (rte->xen_vec);
> > +               }
> > +               else {
> > +                       /* Was already unmasked.  */
> > +                       if (rte->vcpu->domain != current->domain) {
> > +                               spin_unlock_irqrestore(&iosapic_lock,
> > flags);
> > +                               printf ("Oops: GSI %d is reenabled by
> > another"
> > +                                       " domain\n", gsi);
> > +                       }
> > +               }
> > +       }
>
>    Is the domain check before unmasking still missing here?  Seems like
> this block should be almost a mirror image of the masking block.
Well I saw your comment about this.
Only privilegied domain can do a physdev. So currently only dom0 can handle 
interrupts.  For me this check is correct and enough.
With drivers domain (which are not here), I think any drivers domain can 
enable any interrupt. We may add checks but for me this is future work.

Do you have a specific check in mind ?

Thank you for your comments,
Tristan.



_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel