Re: [Xen-devel] [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.17 at 19:28, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, Mar 27, 2017 at 09:59:42AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 12:18, <roger.pau@xxxxxxxxxx> wrote:
>> > @@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
>> >      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> >      unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>> >  
>> > -    ASSERT(isa_irq <= 15);
>> > +    ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>> >  
>> >      spin_lock(&d->arch.hvm_domain.irq_lock);
>> >  
>> > @@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
>> >      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> >      unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>> >  
>> > -    ASSERT(isa_irq <= 15);
>> > +    ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>> 
>> Not sure about these two - perhaps they'd better be
>> BUILD_BUG_ON()s for the DomU side, and Dom0 should never
>> be allocated less than 16?
> 
> Hm, I could add:
> 
> BUILD_BUG_ON(VIOAPIC_NUM_PINS < 16);
> 
> But this is going to make it harder to remove VIOAPIC_NUM_PINS later on.

As said elsewhere, those remaining uses could/should become
ARRAY_SIZE().

>> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
>> >             (uint32_t) hvm_irq->isa_irq.pad[0], 
>> >             hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
>> >             hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
>> > -    for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
>> > +    for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 
>> > 8 )
>> >          printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" 
>> > %2.2"PRIu8
>> >                 " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
>> >                 i, i+7,
>> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
>> >                 hvm_irq->gsi_assert_count[i+5],
>> >                 hvm_irq->gsi_assert_count[i+6],
>> >                 hvm_irq->gsi_assert_count[i+7]);
>> > +    if ( i != hvm_irq->nr_gsis )
>> > +    {
>> > +        printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
>> > +        for ( ; i < hvm_irq->nr_gsis; i++)
>> > +            printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
>> > +        printk("\n");
>> > +    }
>> 
>> I realize you've just copied this from existing code, but what
>> advantage does %2.2 have over just %2 here?
> 
> Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
> would you rather prefer a separate patch?

Well, if you also want to change the existing ones, then a separate
patch would be preferred. As to %2 vs %3 - how would the latter
be any better if we want/need to change the type from u8 anyway?

>> > @@ -545,6 +554,9 @@ static int irq_save_pci(struct domain *d, 
>> > hvm_domain_context_t *h)
>> >      unsigned int asserted, pdev, pintx;
>> >      int rc;
>> >  
>> > +    if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
>> > +        return -EOPNOTSUPP;
>> > +
>> >      spin_lock(&d->arch.hvm_domain.irq_lock);
>> >  
>> >      pdev  = hvm_irq->callback_via.pci.dev;
>> > @@ -592,6 +604,9 @@ static int irq_load_pci(struct domain *d, 
>> > hvm_domain_context_t *h)
>> >      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> >      int link, dev, intx, gsi;
>> >  
>> > +    if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
>> > +        return -EOPNOTSUPP;
>> 
>> Why do you add these checks here?
> 
> Because there's a:
> 
>     for ( gsi = 0; gsi < VIOAPIC_NUM_PINS; gsi++ )
>         hvm_irq->gsi_assert_count[gsi] = 0;
> 
> A little bit below.

True for the load side, but I can't see anything like that for save.

> I can change that to use nr_gsis instead and remove those
> checks (both here and in irq_save_pci).

Please do, there's no harm using the dynamic upper bound in
that case.

>> > @@ -89,13 +77,27 @@ struct hvm_irq {
>> >      u8 round_robin_prev_vcpu;
>> >  
>> >      struct hvm_irq_dpci *dpci;
>> > +
>> > +    /*
>> > +     * Number of wires asserting each GSI.
>> > +     *
>> > +     * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this 
>> > space
>> > +     * except ISA IRQ 0, which is connected to GSI 2.
>> > +     * PCI links map into this space via the PCI-ISA bridge.
>> > +     *
>> > +     * GSIs 16+ are used only be PCI devices. The mapping from PCI device 
>> > to
>> > +     * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
>> > +     */
>> > +    unsigned int nr_gsis;
>> > +    u8 gsi_assert_count[];
>> >  };
>> 
>> I'm afraid at the same time (or in a later patch) the type of this array
>> (and maybe also the type of pci_link_assert_count[]) will need to be
>> widened.
> 
> This one seems to be fine for PVH Dom0 (you will have to look at the next
> series), but I specifically avoid touching pci_link_assert_count. Again, 
> better
> to comment this on the next patch series that actually implements the 
> interrupt
> binding. This is needed because code in vioapic makes use of
> gsi_assert_count.

Well, we can certainly discuss this there (whenever I get to look at
it, as that's not 4.9 stuff now anymore), but for now I don't see the
connection between my comment and your reply.

Jan

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