Re: [Xen-devel] [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.17 at 12:52, <roger.pau@xxxxxxxxxx> wrote:
> +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi,

const struct domain *d ?

> +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic,

const for both?

> +                            unsigned int pin)
> +{
> +    struct hvm_hw_vioapic *tmp;

And here.

Also wouldn't this function more naturally (i.e. more generally useful)
simply return the base GSI, leaving it to the caller to add the pin
number?

> @@ -157,21 +210,22 @@ static void vioapic_write_redirent(
>          pent->fields.remote_irr = 0;
>      else if ( !ent.fields.mask &&
>                !ent.fields.remote_irr &&
> -              hvm_irq->gsi_assert_count[idx] )
> +              hvm_irq->gsi_assert_count[gsi] )

Neither this nor any earlier patch modified the size of this array, afaics.

> -static void vioapic_write_indirect(struct domain *d, uint32_t val)
> +static void vioapic_write_indirect(struct domain *d, unsigned long addr,
> +                                   uint32_t val)
>  {
> -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> +    struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr);

I think it would be better for the caller to pass the vioapic it
already established (and ASSERT()ed).

> @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>  
>  static int vioapic_save(struct domain *d, hvm_domain_context_t *h)
>  {
> -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> +    struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0);
>  
>      if ( !has_vioapic(d) )
>          return 0;
>  
> +    if ( d->arch.hvm_domain.nr_vioapics != 1 )
> +        return -ENOSYS;
> +
>      if ( vioapic->nr_pins != VIOAPIC_NUM_PINS )
>          return -ENOSYS;

Perhaps merge the two if()s? Otherwise -EOPNOTSUPP again.

>  int vioapic_init(struct domain *d)
>  {
>      if ( !has_vioapic(d) )
> +    {
> +        d->arch.hvm_domain.nr_vioapics = 0;

I don't think this is needed - if anything you may want to again
ASSERT() it being zero.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
>      if ( !has_vioapic(d) )
>          return 0;
>  
> -    redir0 = domain_vioapic(d)->redirtbl[0];
> +    redir0 = domain_vioapic(d, 0)->redirtbl[0];

What if the first IO-APIC has less than 16 pins?

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>  static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>  {
>      struct vcpu *v = pt->vcpu;
> -    unsigned int gsi, isa_irq;
> +    struct hvm_hw_vioapic *vioapic;
> +    unsigned int gsi, isa_irq, pin;
>  
>      if ( pt->source == PTSRC_lapic )
>          return pt->irq;
> @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum 
> hvm_intsrc src)
>                  + (isa_irq & 7));
>  
>      ASSERT(src == hvm_intsrc_lapic);
> -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> +
> +    return vioapic->redirtbl[pin].fields.vector;

Please don't chance de-referencing NULL here and below.

Jan


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