Re: [Xen-devel] [PATCH v3 6/8] x86/vioapic: introduce support for multiple vIO APICS

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.17 at 16:47, <roger.pau@xxxxxxxxxx> wrote:
> +static unsigned int base_gsi(const struct domain *d,
> +                             const struct hvm_vioapic *vioapic)
> +{
> +    const struct hvm_vioapic *tmp;
> +    unsigned int base_gsi = 0, nr_vioapics = d->arch.hvm_domain.nr_vioapics;
> +
> +    for ( tmp = domain_vioapic(d, 0); --nr_vioapics && tmp != vioapic; tmp++ 
> )

I don't think tmp++ is correct here - you need to use
domain_vioapic(d, i) on every iteration.

> @@ -361,64 +416,71 @@ static void vioapic_deliver(struct hvm_vioapic 
> *vioapic, int irq)
>  
>  void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
>  {
> -    struct hvm_vioapic *vioapic = domain_vioapic(d);
> +    unsigned int pin;
> +    struct hvm_vioapic *vioapic = gsi_vioapic(d, irq, &pin);
>      union vioapic_redir_entry *ent;
>  
>      ASSERT(has_vioapic(d));

Perhaps better make this ASSERT(vioapic) now? Or even bail if this
ends up being NULL? The ASSERT() above comes too late now in
any event.

>  void vioapic_update_EOI(struct domain *d, u8 vector)
>  {
> -    struct hvm_vioapic *vioapic = domain_vioapic(d);
>      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>      union vioapic_redir_entry *ent;
> -    int gsi;
> +    unsigned int i, base_gsi = 0;
>  
>      ASSERT(has_vioapic(d));
> 
>      spin_lock(&d->arch.hvm_domain.irq_lock);
>  
> -    for ( gsi = 0; gsi < vioapic->nr_pins; gsi++ )
> +    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
>      {
> -        ent = &vioapic->redirtbl[gsi];
> -        if ( ent->fields.vector != vector )
> -            continue;
> -
> -        ent->fields.remote_irr = 0;
> +        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> +        unsigned int j;
>  
> -        if ( iommu_enabled )
> +        for ( j = 0; j < vioapic->nr_pins; j++ )

Please use pin instead of j.

> @@ -426,12 +488,13 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>  
>  static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
>  {
> -    struct hvm_vioapic *s = domain_vioapic(d);
> +    struct hvm_vioapic *s = domain_vioapic(d, 0);
>  
>      if ( !has_vioapic(d) )
>          return 0;

This check needs to be ahead of domain_vioapic() use - I'd expect
d->vioapic to be NULL in such a case, and the macro dereferences
it. Please double check other uses.

> @@ -454,32 +518,68 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, 
> ioapic_load, 1, HVMSR_PER_DOM);
>  
>  void vioapic_reset(struct domain *d)
>  {
> -    struct hvm_vioapic *vioapic = domain_vioapic(d);
> -    uint32_t nr_pins = vioapic->nr_pins;
> -    int i;
> +    unsigned int i;
>  
>      if ( !has_vioapic(d) )
> +    {
> +        ASSERT(!d->arch.hvm_domain.nr_vioapics);
>          return;
> +    }
>  
> -    memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> -    vioapic->domain = d;
> -    vioapic->nr_pins = nr_pins;
> -    for ( i = 0; i < nr_pins; i++ )
> -        vioapic->redirtbl[i].fields.mask = 1;
> -    vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
> +    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
> +    {
> +        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> +        unsigned int j, nr_pins = vioapic->nr_pins;
> +
> +        memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> +        for ( j = 0; j < nr_pins; j++ )
> +            vioapic->redirtbl[j].fields.mask = 1;

It's not as relevant here, but s/j/pin/ would again be better imo.

> +        ASSERT(!i);
> +        vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
> +                                VIOAPIC_MEM_LENGTH * i;

With the ASSERT() above, please drop the (wrong, as pointed out
before) dependency on i of this expression.

> @@ -91,13 +92,22 @@ 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);
> +    if ( !vioapic )
> +    {
> +        gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", 
> gsi);

Unless v == current at all times, gdprintk() is not suitable here, and
you'd rather want to include d->domain_id in what is being logged.

> @@ -110,9 +120,16 @@ static int pt_irq_masked(struct periodic_time *pt)
>      isa_irq = pt->irq;
>      gsi = hvm_isa_irq_to_gsi(isa_irq);
>      pic_imr = v->domain->arch.hvm_domain.vpic[isa_irq >> 3].imr;
> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> +    if ( !vioapic )
> +    {
> +        gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", 
> gsi);

Same here.

> --- a/xen/include/asm-x86/hvm/vioapic.h
> +++ b/xen/include/asm-x86/hvm/vioapic.h
> @@ -57,8 +57,10 @@ struct hvm_vioapic {
>  };
>  
>  #define hvm_vioapic_size(cnt) offsetof(struct hvm_vioapic, redirtbl[cnt])
> -#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic)
> +#define domain_vioapic(d, i) ((d)->arch.hvm_domain.vioapic[i])
>  #define vioapic_domain(v) ((v)->domain)
> +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
> +                                unsigned int *pin);

Please don't munge this together with macros, but with other
function declarations (or at least add a blank line in between).

Jan

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