|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS
>>> On 27.03.17 at 12:18, <roger.pau@xxxxxxxxxx> wrote:
> Changes since v1:
> - Constify some parameters.
Considering this I'm surprised the first thing I have to ask is ...
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -44,6 +44,54 @@
>
> static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq);
>
> +static struct hvm_vioapic *addr_vioapic(struct domain *d, unsigned long addr)
... const? The more that you have it like this ...
> +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
... here.
> + unsigned int *base_gsi)
All callers of the function really want the pin number rather than
the base GSI - why don't you make the function provide that
instead?
> +{
> + unsigned int i;
> + *base_gsi = 0;
Blank line between declaration(s) and statement(s) please.
> +static unsigned int pin_gsi(const struct domain *d,
> + const struct hvm_vioapic *vioapic,
> + unsigned int pin)
> +{
> + const struct hvm_vioapic *tmp;
> + unsigned int base_gsi = 0;
> +
> + for ( tmp = domain_vioapic(d, 0); tmp != vioapic; tmp++ )
> + base_gsi += tmp->nr_pins;
> +
> + return base_gsi + pin;
> +}
Didn't we settle on having a function base_gsi(), with callers
adding in the pin number as needed? The only reason the function
here takes pin as a parameter is this final addition.
Also shouldn't this function somehow guard itself against
overrunning the array?
> @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void)
> return pt_active(¤t->domain->arch.vpit.pt0);
> }
>
> -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
> +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin)
Occasionally we pass around negative IRQ numbers, but if this
really is a pin number, I think the type wants changing to unsigned
int - the more that it is being used as an array index below.
> @@ -454,33 +517,70 @@ 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;
> + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
> + VIOAPIC_MEM_LENGTH * i;
> + vioapic->id = i;
> + vioapic->nr_pins = nr_pins;
> + vioapic->domain = d;
> + }
> +}
Is this function validly reachable for Dom0? If not, better leave it
alone (adding a nr_vioapics == 1 check), as at least the base
address calculation looks rather suspicious (there shouldn't be
multiple IO-APICs on a single page). If so, that same memory
address calculation is actively wrong in the Dom0 case.
> +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics)
> +{
> + unsigned int i;
> +
> + for ( i = 0; i < nr_vioapics; i++)
> + {
> + if ( domain_vioapic(d, i) == NULL )
> + break;
Is this if() really needed?
> int vioapic_init(struct domain *d)
> {
> + unsigned int i, nr_vioapics = 1;
> +
> if ( !has_vioapic(d) )
> + {
> + ASSERT(!d->arch.hvm_domain.nr_vioapics);
> return 0;
> + }
>
> if ( (d->arch.hvm_domain.vioapic == NULL) &&
> ((d->arch.hvm_domain.vioapic =
> - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
> + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
> return -ENOMEM;
>
> - d->arch.hvm_domain.vioapic->domain = d;
> - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
> + for ( i = 0; i < nr_vioapics; i++ )
> + {
> + if ( (domain_vioapic(d, i) =
> + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
> + {
> + vioapic_free(d, nr_vioapics);
> + return -ENOMEM;
> + }
> + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
> + }
> +
> + d->arch.hvm_domain.nr_vioapics = nr_vioapics;
> vioapic_reset(d);
These adjustments again don't look right for nr_vioapics > 1, so I
wonder whether again this function wouldn't better be left alone.
Alternatively, if Dom0 needs to come here, a comment is going to
be needed to explain the (temporary) wrong setting of nr_pins.
> @@ -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, &base_gsi);
> + if ( !vioapic )
> + {
> + gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n",
> gsi);
> + domain_crash(v->domain);
> + return -1;
> + }
Now that you have this check, ...
> @@ -110,9 +120,10 @@ 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, &base_gsi);
>
> return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v))
> &&
> - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask);
> + vioapic->redirtbl[gsi - base_gsi].fields.mask);
> }
... why is there still none here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |